Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ui): support scaling of resources in UI (#15505) #20097

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

ratulbasak
Copy link

@ratulbasak ratulbasak commented Sep 25, 2024

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

@ratulbasak ratulbasak requested a review from a team as a code owner September 25, 2024 08:08
Copy link

bunnyshell bot commented Sep 25, 2024

🔴 Preview Environment stopped on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

  • 🔵 /bns:start to start the environment
  • 🚀 /bns:deploy to redeploy the environment
  • /bns:delete to remove the environment

Copy link

bunnyshell bot commented Sep 25, 2024

✅ Preview Environment created on Bunnyshell but will not be auto-deployed

See: Environment Details

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

ratulbasak and others added 16 commits September 25, 2024 10:49
Signed-off-by: ratulbasak <[email protected]>
* Add state to pkce flow

Signed-off-by: Jungho Son <[email protected]>

* Call unset

Signed-off-by: Jungho Son <[email protected]>

---------

Signed-off-by: Jungho Son <[email protected]>
Signed-off-by: ratulbasak <[email protected]>
* docs: add link to example repo creds yaml

Signed-off-by: Nicholas Morey <[email protected]>

* docs: add project to repo creds examples

Signed-off-by: Nicholas Morey <[email protected]>

---------

Signed-off-by: Nicholas Morey <[email protected]>
Signed-off-by: ratulbasak <[email protected]>
Without the extra line break, the paragraph was rendered as part of the Kustomize application manifest.

Signed-off-by: Allan M. de Azevedo <[email protected]>
Signed-off-by: ratulbasak <[email protected]>
…18441)

* feat: option to disable writing k8s events

optioned to write logs for k8s events.
Each is passed as an environment variable and defaults to true,
disabling it requires explicitly setting the option to false.

Signed-off-by: Jack-R-lantern <[email protected]>

* feat: option to disable writing k8s events

fix unit test
- application_test
- applicationset_test
- project_test
- appcontroller_tes
- audit_logger_test

Signed-off-by: Jack-R-lantern <[email protected]>

* rebase

Signed-off-by: Jack-R-lantern <[email protected]>

---------

Signed-off-by: Jack-R-lantern <[email protected]>
Signed-off-by: ratulbasak <[email protected]>
…roj#20059)

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.66.2 to 1.67.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.66.2...v1.67.0)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: ratulbasak <[email protected]>
* fix broken link

Signed-off-by: nitishfy <[email protected]>

* fix broken link

Signed-off-by: nitishfy <[email protected]>

---------

Signed-off-by: nitishfy <[email protected]>
Signed-off-by: ratulbasak <[email protected]>
Signed-off-by: CI <[email protected]>
Co-authored-by: CI <[email protected]>
Signed-off-by: ratulbasak <[email protected]>
… plugin-based applications (argoproj#19209)

* chore: transmit manifest-generate-path resources to the cmp-server for plugin-based applications
Signed-off-by: Javier Solana <[email protected]>

* use SecureJoin
Signed-off-by: Javier Solana <[email protected]>

* make cmp manifests generation using manifest generate path annotation configurable by environment variable
Signed-off-by: Javier Solana <[email protected]>

* fix missing doc running codegen-local
Signed-off-by: Javier Solana <[email protected]>

* set reposerver.plugin.enable.manifests.generation.using.annotations false by default
Signed-off-by: Javier Solana <[email protected]>

* define ARGOCD_REPO_SERVER_PLUGIN_ENABLE_GENERATE_MANIFESTS_USING_MANIFEST_GENERATE_PATHS_ANNOTATION properly
Signed-off-by: Javier Solana <[email protected]>

* define ARGOCD_REPO_SERVER_PLUGIN_ENABLE_GENERATE_MANIFESTS_USING_MANIFEST_GENERATE_PATHS_ANNOTATION properly
Signed-off-by: Javier Solana <[email protected]>

* Fix conflict
Signed-off-by: Javier Solana <[email protected]>

* autogenerate install manifests
Signed-off-by: Javier Solana <[email protected]>

* add note about common root path calculation for manifest paths annotation
Signed-off-by: Javier Solana <[email protected]>

* log common root path calculated
Signed-off-by: Javier Solana <[email protected]>

* app path must be the lower common path
Signed-off-by: Javier Solana <[email protected]>

* tweaks

Signed-off-by: Michael Crenshaw <[email protected]>

---------

Signed-off-by: Michael Crenshaw <[email protected]>
Co-authored-by: Javier Solana <[email protected]>
Co-authored-by: Michael Crenshaw <[email protected]>
Signed-off-by: ratulbasak <[email protected]>
Signed-off-by: ratulbasak <[email protected]>
…pgrading path-to-regexp from 1.8.0 to 1.9.0 (argoproj#20087)

Signed-off-by: Cheng Fang <[email protected]>
Signed-off-by: ratulbasak <[email protected]>
Copy link

codecov bot commented Sep 25, 2024

Codecov Report

Attention: Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 55.96%. Comparing base (29c59ab) to head (668d042).

Files with missing lines Patch % Lines
util/lua/lua.go 62.50% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #20097   +/-   ##
=======================================
  Coverage   55.96%   55.96%           
=======================================
  Files         322      322           
  Lines       44717    44720    +3     
=======================================
+ Hits        25025    25027    +2     
- Misses      17095    17096    +1     
  Partials     2597     2597           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -1,3 +1,4 @@
local actions = {}
actions["restart"] = {}
actions["scale"] = {["defaultValue"] = tostring(obj.spec.replicas), ["hasParameters"] = true, ["errorMessage"] = "Enter any valid number", ["regexp"]= "^[0-9][0-9]*$"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is + not supported?

I'd expect a space before =.

Is \d defined to more than [0-9]?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added space and Updated regex

local os = require("os")

obj.spec.replicas = tonumber(scale)
return obj
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add newline at end of file

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added new line

@@ -7,4 +7,5 @@ if obj.spec.paused ~= nil then
actions["pause"] = {paused}
end
actions["resume"] = {["disabled"] = not(paused)}
actions["scale"] = {["defaultValue"] = tostring(obj.spec.replicas), ["hasParameters"] = true, ["errorMessage"] = "Enter any valid number", ["regexp"] = "^[0-9]*$", }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this feature also extended to Argo Rollouts resources which is based on K8s Deployment? Can this be done in this PR?

Probably worth documenting this feature and set expectations when using HPA or application auto-sync is enabled.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feature in argo rollouts will be done in a separate PR

@ratulbasak
Copy link
Author

Hello @crenshaw-dev, please look into this PR - an enhancement feature: #15505 which was already approved about a year ago. Could you please help me on how to move forward?

Copy link
Member

@reggie-k reggie-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the PR!
Since this change is not just adding another custom action, but adding new (and useful!) functionality to custom action mechanism along with API changes, could you add the following tests:

  • Something like resource_customizations/apps/DaemonSet/actions/testdata/daemonset-restarted.yaml, for every type of deployment for which the scale action is applicable

  • util/lua/lua_test.go (one example for tests that can be relevant are those that use the cronJobObjYaml, those tests are also testing a change in the custom action mechanism)

  • cmd/argocd/commands/admin/settings_test.go

  • server/application/application_test.go

  • util/lua/custom_actions_test.go (a test with ResourceActionParameters)

And could you add relevant documentation with an example how to use this feature?

@ratulbasak
Copy link
Author

Thanks a lot for the PR! Since this change is not just adding another custom action, but adding new (and useful!) functionality to custom action mechanism along with API changes, could you add the following tests:

  • Something like resource_customizations/apps/DaemonSet/actions/testdata/daemonset-restarted.yaml, for every type of deployment for which the scale action is applicable
  • util/lua/lua_test.go (one example for tests that can be relevant are those that use the cronJobObjYaml, those tests are also testing a change in the custom action mechanism)
  • cmd/argocd/commands/admin/settings_test.go
  • server/application/application_test.go
  • util/lua/custom_actions_test.go (a test with ResourceActionParameters)

And could you add relevant documentation with an example how to use this feature?

Thank you for providing further steps.
Should I use the same resource_customizations/apps/DaemonSet/actions/testdata/daemonset.yaml file as an input?
Where should I add the documentation?

Copy link
Contributor

@todaywasawesome todaywasawesome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ratulbasak I would add an entry under the User Guide. https://argo-cd.readthedocs.io/en/stable/user-guide/

I think we should also add a CLI option but it shouldn't block the PR from being merged.

@crenshaw-dev
Copy link
Member

/bns:deploy

@reggie-k
Copy link
Member

reggie-k commented Oct 6, 2024

resource_customizations/apps/DaemonSet/actions/testdata/

Sorry, actually thinking about it again, a DaemonSet is probably not relevant for scaling in/out since relies on a node selector, so probably this action is relevant only for Deployment and StatefulSet, and I think that you can use the existing inputs for both, along with adding new outputs with changed replicas for scaling out and for scaling in.

@ratulbasak
Copy link
Author

resource_customizations/apps/DaemonSet/actions/testdata/

Sorry, actually thinking about it again, a DaemonSet is probably not relevant for scaling in/out since relies on a node selector, so probably this action is relevant only for Deployment and StatefulSet, and I think that you can use the existing inputs for both, along with adding new outputs with changed replicas for scaling out and for scaling in.

I have added the doc in user-guide and also included Deployment and StatefulSet manifests

@crenshaw-dev crenshaw-dev changed the title chore: 15505 support scaling of resources in UI feat(ui): support scaling of resources in UI (#15505) Oct 11, 2024
@@ -1,3 +1,4 @@
local actions = {}
actions["restart"] = {}
actions["scale"] = {["defaultValue"] = tostring(obj.spec.replicas), ["hasParameters"] = true, ["errorMessage"] = "Enter any valid number", ["regexp"] = "^[0-9]*$"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the scale action is not relevant for Daemon Sets, since they operate on NodeSelectors.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed DaemonSet scale action

util/lua/lua.go Outdated
@@ -60,7 +61,7 @@ type VM struct {
UseOpenLibs bool
}

func (vm VM) runLua(obj *unstructured.Unstructured, script string) (*lua.LState, error) {
func (vm VM) runLua(obj *unstructured.Unstructured, script string, resourceActionParameters []*applicationpkg.ResourceActionParameters) (*lua.LState, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a new method, something like runLuaWithResourceActionParameters, be added? And the new method would invoke the existing one and then operate on the input parameters afterwards?
This way most places in the code, that don't require the parameters, can remain unchanged and then only the new code which needs the params would invoke the new function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for the suggestion, added a new method that invokes the existing one.

@@ -600,7 +600,7 @@ argocd admin settings resource-overrides action run /tmp/deploy.yaml restart --a
action, err := luaVM.GetResourceAction(&res, action)
errors.CheckError(err)

modifiedRes, err := luaVM.ExecuteResourceAction(&res, action.ActionLua)
modifiedRes, err := luaVM.ExecuteResourceAction(&res, action.ActionLua, nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resource parameters should be passed here, if received from the command line.
An e2e argocd cli test would ensure the cli works correctly with the new functionality.

Copy link
Author

@ratulbasak ratulbasak Oct 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please help on the following error in end-to-end test? seems to be a lock on the Git repository, preventing the application from being created successfully.
time="2024-10-20T21:54:36Z" level=error msg="`../../dist/argocd app create test-installation-id --repo file:///tmp/argo-e2e/testdata.git --dest-server https://kubernetes.default.svc --path guestbook --project default --dest-namespace argocd-e2e--test-installation-id-yrgcy --plaintext --server 127.0.0.1:8088 --auth-token *** --insecure` failed exit status 20: time=\"2024-10-20T21:54:35Z\" level=fatal msg=\"rpc error: code = InvalidArgument desc = application spec for test-installation-id is invalid: InvalidSpecError: Unable to generate manifests in guestbook: rpc error: code = Unknown desc = failed to initialize repository resources: rpc error: code = Internal desc = Failed to fetch default: `git fetch origin --tags --force --prune` failed exit status 1: error: cannot lock ref 'refs/remotes/origin/master': Unable to create '<path to cached source>/.git/refs/remotes/origin/master.lock': File exists.\\n\\nAnother git process seems to be running in this repository

@@ -0,0 +1,4 @@
local os = require("os")
Copy link
Member

@reggie-k reggie-k Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In ArgoCD, there are automatic tests that run for each built-in resource action.
Those tests are run in TestLuaResourceActionsScript in util/lua/custom_actions_test.go
Those tests expect an input manifest, and and output manifest, that represents the output of the action when run on the input manifest.
Look at resource_customizations/apps/Deployment/actions/action_test.yaml , it has an example of input and output files for the action test, you would need to add a similar for Deployment + StatefulSet. The idea is that you provide a Deployment manifest with replicas set to x as an input, and provide a Deployment manifest with replicas set to y as an output. The automatic test will succeed if your custom action yields the output manifest.
You don't have to do this twice for scale-in and scale-out, one example of a scale change is enough for Deployment and one for StatefulSet.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added scale actions in resource_customizations/apps/Deployment/actions/action_test.yaml and resource_customizations/apps/StatefulSet/actions/action_test.yaml files and also tests in util/lua/custom_actions_test.go

IconClass string `json:"iconClass,omitempty" protobuf:"bytes,4,opt,name=iconClass"`
DisplayName string `json:"displayName,omitempty" protobuf:"bytes,5,opt,name=displayName"`
Name string `json:"name,omitempty" protobuf:"bytes,1,opt,name=name"`
Params []ResourceActionParam `json:"params,omitempty" protobuf:"bytes,2,rep,name=params"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, there is an existing Params field, and looking at ResourceActionParam, it has most of the needed fields, I checked and it is used in the util/lua/lua_test.go, and there is even a test there for an action scale. Can you maybe make use of this field and check what this existing scale test is used for?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.