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

Documentation for dynamic scaling of Argo CD application controller. #67913

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

masauso-lungu
Copy link
Contributor

@masauso-lungu masauso-lungu commented Nov 15, 2023

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 15, 2023
@ocpdocs-previewbot
Copy link

ocpdocs-previewbot commented Nov 15, 2023

🤖 Updated build preview is available at:
https://67913--docspreview.netlify.app

Build log: https://circleci.com/gh/ocpdocs-previewbot/openshift-docs/37291

Copy link
Contributor

@eromanova97 eromanova97 left a comment

Choose a reason for hiding this comment

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

Hello @masauso-lungu I just saw your message about this PR not being finished, but as I already started my review, I will post my initial comments here so that it does not go to waste. I can then do the full review once the PR is finished. Thank you!

Also, the preview link you put in the PR did not work for me, so here is the link if you could please update it :) thanks!
https://67913--docspreview.netlify.app/openshift-gitops/latest/argocd_instance/argo-cd-cr-component-properties#gitops-argo-cd-dynamic-scaling_argo-cd-cr-component-properties

modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 4, 2023
@eromanova97
Copy link
Contributor

Notes from the meeting:
We discussed with @masauso-lungu what would be the best solution to include the CLI steps to enable dynamic scaling of shards.

My suggestion is to not include it as an optional step 6, because I imagine the user would begin the procedure and go from step 1 to step 5 without noticing that there is an alternative solution added as step 6.

There are two solutions that come to mind:

  1. First would be to create two procedures, one for web console and second for CLI
    But, as @masauso-lungu correctly pointed out, a lot of the same information would be repeated in these two procedures.

  2. The second solution would be to change the procedure as follows:

    • we would have two main bullet points, something like:
      • To enable dynamic shard scaling in web console, follow the steps below.
        1.
        2.
        etc. (that would be the steps that are currently included as steps 1. - 5.)

      • To enable dynamic shard scaling using CLI, run the following command:
        $ oc edit argocd <argocd-instance> -n <namespace> (command from step 6)
        Example
        <add example of the edited YAML file, same as in step 4 of the current procedure>
        Example output
        $ argocd.argoproj.io/<argocd-instance> edited

I hope my description is clear enough, @masauso-lungu please let me know if you want some example of what I mean, but I hope I was able to explain it nicely during our conversation :D Thank you!

@masauso-lungu
Copy link
Contributor Author

Notes from the meeting: We discussed with @masauso-lungu what would be the best solution to include the CLI steps to enable dynamic scaling of shards.

My suggestion is to not include it as an optional step 6, because I imagine the user would begin the procedure and go from step 1 to step 5 without noticing that there is an alternative solution added as step 6.

There are two solutions that come to mind:

  1. First would be to create two procedures, one for web console and second for CLI
    But, as @masauso-lungu correctly pointed out, a lot of the same information would be repeated in these two procedures.

  2. The second solution would be to change the procedure as follows:

    • we would have two main bullet points, something like:

      • To enable dynamic shard scaling in web console, follow the steps below.
        1.
        2.
        etc. (that would be the steps that are currently included as steps 1. - 5.)
      • To enable dynamic shard scaling using CLI, run the following command:
        $ oc edit argocd <argocd-instance> -n <namespace> (command from step 6)
        Example
        <add example of the edited YAML file, same as in step 4 of the current procedure>
        Example output
        $ argocd.argoproj.io/<argocd-instance> edited

I hope my description is clear enough, @masauso-lungu please let me know if you want some example of what I mean, but I hope I was able to explain it nicely during our conversation :D Thank you!

Thank you Eliska for the summary. The new commit will be best on the second solution as agreed.

@masauso-lungu masauso-lungu marked this pull request as ready for review December 4, 2023 19:00
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 4, 2023
@masauso-lungu
Copy link
Contributor Author

Hello @ishitasequeira , this PR is ready for reviews.
The files directly linked to this issue are:

  1. argocd_instance/argo-cd-cr-component-properties.adoc
  2. modules/gitops-argo-cd-dynamic-scaling.adoc

Other files are imported as my PR's content was outdated.

On the section for using the command line interface, I have adjusted the commands to make it easier to verify the cofiguration. Let me know if everything checks out.

modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
Copy link
Contributor

@eromanova97 eromanova97 left a comment

Choose a reason for hiding this comment

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

Hello @masauso-lungu I left some comments so please take a look. I am not feeling the greatest, so I might have missed something or suggested something wrongly. So once you implement the changes or answer to my suggestions, please ping me and I look at the issue once again. Thank you for understanding!

modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
@masauso-lungu
Copy link
Contributor Author

Hello @ishitasequeira kindly review the content of this PR.

Copy link
Contributor

@reginapizza reginapizza left a comment

Choose a reason for hiding this comment

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

just a couple typo comments but otherwise LGTM

modules/gitops-argo-cd-properties.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-properties.adoc Outdated Show resolved Hide resolved
@masauso-lungu
Copy link
Contributor Author

/label sme-review-done

@openshift-ci openshift-ci bot added the sme-review-done Service Delivery issues that have been reviewed by SRE, PMs, Engineering, QE, etc. label Jan 2, 2024
@varshab1210
Copy link

LGTM, thanks

@masauso-lungu
Copy link
Contributor Author

LGTM, thanks

Thank you @varshab1210

@masauso-lungu
Copy link
Contributor Author

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Jan 2, 2024
@masauso-lungu
Copy link
Contributor Author

/label peer-review-needed

@openshift-ci openshift-ci bot added the peer-review-needed Signifies that the peer review team needs to review this PR label Jan 2, 2024
@nalhadef
Copy link
Contributor

nalhadef commented Jan 2, 2024

/label peer-review-in-progress
/remove-label peer-review-needed

@openshift-ci openshift-ci bot added peer-review-in-progress Signifies that the peer review team is reviewing this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Jan 2, 2024
@Srivaralakshmi
Copy link
Member

@ekristova Great! Because both topics are advanced level, they must reside under the Declarative cluster configuration chapter. Also, can you please do a thorough review of this PR? I see scope for improvements and it will be a good experience for both of you. Thanks!

@masauso-lungu please sync up with @ekristova. TY!

@masauso-lungu
Copy link
Contributor Author

Hello @masauso-lungu @Srivaralakshmi I actually have an idea about how to include this. I am working on it in my PR: https://github.com/openshift/openshift-docs/pull/69199/files#diff-4c57a7ed081d06dddb5c1f711b55565a1a847260a3311ad3bf911ea3a0e6af49R13

You can see I added a note where would the sharding section go. I am trying to connect with SME to have the changes addressed as soon as possible, but I had no luck so far so I am not sure when this is going to be merged. So in the meantime, @masauso-lungu can you contact me so that we can discuss this? Thank you!

Thank you @ekristova .....

@openshift-ci openshift-ci bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 4, 2024
Copy link
Contributor

@eromanova97 eromanova97 left a comment

Choose a reason for hiding this comment

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

Hello @masauso-lungu to make it as easy as possible for you, I quickly added comments explaining what needs to be changed in structure. I will do a thorough review later but I wanted to give you these tips as soon as possible so that you are not blocked :) Thank you!

modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
Copy link
Contributor

@eromanova97 eromanova97 left a comment

Choose a reason for hiding this comment

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

I went through the PR thoroughly now and I had some more suggestions, I hope it is helpful. I know that it must be frustrating that we are changing things again, but hopefully these are the last big changes that should happen :) Thank you!

modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-properties.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-properties.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling.adoc Outdated Show resolved Hide resolved
Copy link
Member

@Srivaralakshmi Srivaralakshmi left a comment

Choose a reason for hiding this comment

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

@masauso-lungu Thanks for the good work! Left a few suggestions for you to consider.

modules/gitops-argo-cd-properties.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-properties.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-properties.adoc Outdated Show resolved Hide resolved
@eromanova97
Copy link
Contributor

eromanova97 commented Jan 18, 2024

Hello @masauso-lungu there is one more small adjustment that needs to be done, it is based on this peer review comment I received: #69199 (comment) so please create the modules the same way as it is in my PR now.
Other than that, my PR is currently going through the merge review so once it is merged, you can rebase and add your sections under the round-robin module, as I commented here in the assembly. Thank you!

Edit: my issue is now merged :)

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 18, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 18, 2024
@openshift-ci openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 18, 2024
Copy link
Contributor

@eromanova97 eromanova97 left a comment

Choose a reason for hiding this comment

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

Hello @masauso-lungu good work! I have just very small suggestions for you, otherwise looks good to me! :)

Also, I have just one additional question, I was just curious, if we could utilize checking the logs for the verification of this feature, similarly to the round-robin algorithm procedure. It is just something I wondered about :) Thank you!

modules/gitops-argo-cd-dynamic-scaling-on-web-console.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling-on-web-console.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling-on-web-console.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling-on-web-console.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling-on-web-console.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling-using-cli.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling-using-cli.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling-using-cli.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling-using-cli.adoc Outdated Show resolved Hide resolved
modules/gitops-argo-cd-dynamic-scaling-using-cli.adoc Outdated Show resolved Hide resolved
@masauso-lungu
Copy link
Contributor Author

/label merge-review-needed

@openshift-ci openshift-ci bot added the merge-review-needed Signifies that the merge review team needs to review this PR label Jan 19, 2024
@skrthomas skrthomas added merge-review-in-progress Signifies that the merge review team is reviewing this PR and removed merge-review-needed Signifies that the merge review team needs to review this PR labels Jan 19, 2024
Copy link
Contributor

@skrthomas skrthomas left a comment

Choose a reason for hiding this comment

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

I have a few style things I noted you probably should consider/address before merging. Just ping me on Slack when you have them incorporated or if you have any questions :)

@skrthomas skrthomas merged commit d193741 into openshift:gitops-docs Jan 23, 2024
1 check passed
@skrthomas
Copy link
Contributor

/cherrypick gitops-docs-1.10

@skrthomas
Copy link
Contributor

/cherrypick gitops-docs-1.11

@openshift-cherrypick-robot

@skrthomas: new pull request created: #70694

In response to this:

/cherrypick gitops-docs-1.10

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-cherrypick-robot

@skrthomas: new pull request created: #70695

In response to this:

/cherrypick gitops-docs-1.11

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-review-in-progress Signifies that the merge review team is reviewing this PR peer-review-done Signifies that the peer review team has reviewed this PR qe-approved Signifies that QE has signed off on this PR size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sme-review-done Service Delivery issues that have been reviewed by SRE, PMs, Engineering, QE, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet