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

csi-isilon: second csi-metadata-retriever instance eliminated #453

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

alexemc
Copy link
Contributor

@alexemc alexemc commented Jun 14, 2024

Do not install additional instance of retriever when Encryption is enabled.

Is this a new chart?

No

What this PR does / why we need it:

Do not install additional instance of retriever when Encryption is enabled.

Which issue(s) is this PR associated with:

1309

Special notes for your reviewer:

Checklist:

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • Chart Version bumped
  • Title of the PR starts with the chart name (e.g. [charts_dir/mychartname]) if applicable

donatwork
donatwork previously approved these changes Jun 14, 2024
@donatwork donatwork self-requested a review June 14, 2024 21:01
Copy link
Contributor

@nitesh3108 nitesh3108 left a comment

Choose a reason for hiding this comment

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

metadata-retriever is mandatory sidecar to create volume, I'm not sure why this check is in place if encryption is enabled ?

@gallacher
Copy link
Collaborator

gallacher commented Jun 17, 2024

@alexemc, should this be merged into the release branch instead? release-v1.11.0

@alexemc
Copy link
Contributor Author

alexemc commented Jun 17, 2024

@alexemc, should this be merged into the release branch instead? release-v1.11.0

@gallacher I'm not familiar with the process in CSM. Should I merge it to release-v1.11.0 instead of main?

@alexemc
Copy link
Contributor Author

alexemc commented Jun 17, 2024

metadata-retriever is mandatory sidecar to create volume, I'm not sure why this check is in place if encryption is enabled ?

There is also a loop on line 184, so this part of yalm is rendered twice - first time with encrypted=false, second time (provided, that encryption module is enabled) with encrypted=true. The bug was that the retriever container was included in both passes of the loop resulting in two instances of the container, which is not needed and fails to work. With the fix in this PR, the retriever is only included once, during the first pass of the loop. Hope this explains the change.

@shaynafinocchiaro
Copy link
Contributor

@alexemc, should this be merged into the release branch instead? release-v1.11.0

@gallacher I'm not familiar with the process in CSM. Should I merge it to release-v1.11.0 instead of main?

Hi @alexemc any chart changes that go into main kick off the release actions so ideally we should merge to a release branch. Please merge to the release-v1.11.0 branch. Please also address the failed checks before merging to release-v1.11.0. Thank you!

@alexemc alexemc changed the base branch from main to release-v1.11.0 June 17, 2024 14:50
@alexemc alexemc force-pushed the retriever-with-encryption-fix branch from 1da46fd to 0d06926 Compare June 17, 2024 15:25
@alexemc alexemc force-pushed the retriever-with-encryption-fix branch from 0d06926 to 7681a96 Compare June 17, 2024 15:58
Copy link
Contributor

@JacobGros JacobGros left a comment

Choose a reason for hiding this comment

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

LGTM

@shaynafinocchiaro
Copy link
Contributor

If these linting issues are still relevant https://github.com/dell/helm-charts/actions/runs/9548768257, can you address them as part of your PR? Thanks

@alexemc
Copy link
Contributor Author

alexemc commented Jun 17, 2024

If these linting issues are still relevant https://github.com/dell/helm-charts/actions/runs/9548768257, can you address them as part of your PR? Thanks

This PR was initially supposed to merge to main and there were many failed linter checks (in files I have not even touched), but then I rebased to release-v1.11.0 and all checks succeeded.

@alexemc alexemc merged commit 16bdcba into release-v1.11.0 Jun 17, 2024
4 checks passed
shaynafinocchiaro added a commit that referenced this pull request Jul 29, 2024
* Release 1.10.1 (#401) (#405)

* Fix Helm deployment of vxflexos driver (#397)

* [csm-application-mobility] Revert app mobility version changes (#399)

* Revert app mobility version changes

---------

Co-authored-by: Adarsh Kumar Yadav <[email protected]>
Co-authored-by: KshitijaKakde <[email protected]>
Co-authored-by: gallacher <[email protected]>

* Bump chart driver versions for CSM 1.11 (#407)

* Luna/fix encryption (#404)

* fix conflicts

* Bumping reverseproxy version (#371)

* COSI driver verion bump up (#370)

* CSI-Sidecars version bumpup (#372)

* CSI-Sidecars version bumpup

* Snapshotter patch version update

* app mobility version bump (#374)

* change default value (#381)

* version bumps (#378)

* Updated CSI Metadata Version (#382)

* change to deploy encryption

* Update Chart.yaml

* Update values.yaml

* Update Chart.yaml

* Update values.yaml

---------

Co-authored-by: boyamurthy <[email protected]>
Co-authored-by: Adarsh Kumar Yadav <[email protected]>
Co-authored-by: Rishabh Raj <[email protected]>
Co-authored-by: Aaron Tye <[email protected]>
Co-authored-by: Luna Xu <[email protected]>
Co-authored-by: Luna Xu <[email protected]>
Co-authored-by: shaynafinocchiaro <[email protected]>

* `[charts/csi-unity]` fix: add allowedNetworks parameter (#406)

* fix: add allowedNetworks parameter

* change release version

* [multiple charts] Set default container to driver (#409)

* Remove check on maximum version of kube (#410)

* Bump Observability version (#412)

* Bump the module versions (#417)

* Address linting errors (#450)

* fix linting issues

* revert version changes

* [BUG]: [csi-vxflexos]: Update `controller.yaml` template (#428)

* Release 1.10.1 (#401)

* Fix Helm deployment of vxflexos driver (#397)

* [csm-application-mobility] Revert app mobility version changes (#399)

* Revert app mobility version changes

---------

Co-authored-by: KshitijaKakde <[email protected]>
Co-authored-by: gallacher <[email protected]>

* Update driver/module versions for 10.0.2 (#424)

* Revert "Update driver/module versions for 10.0.2 (#424)" (#425)

* Update controller.yaml

Removed unknown field under 
"spec.template.spec.affinity.nodeSelector"

---------

Co-authored-by: Adarsh Kumar Yadav <[email protected]>
Co-authored-by: KshitijaKakde <[email protected]>
Co-authored-by: gallacher <[email protected]>
Co-authored-by: boyamurthy <[email protected]>
Co-authored-by: shaynafinocchiaro <[email protected]>

* Powermax resiliency changes (#421)

* Do not install additional instance of retriever when Encryption is enabled. (#453)

* Updating latest sidecar versions (#454)

* update usage of allowedNetworks in unity values.yaml (#455)

* Removed warning from Chart (#457)

Add complex version check example back

* Added the patch verb to volumesnapshotcontents/status to fix snapshots creation. (#458)

Co-authored-by: JacobGros <[email protected]>

* [charts/csm-authorization]: CSM Authorization v2 (#459)

* add proxy-server sa

* [csm-authorization]: Add Vault configuration to storage-service (#350)

* Add Storage and CSMRole CRD into Authorization helm chart. (#305)

* add crds

* change group name

* Revert "change group name"

This reverts commit de262a3.

* vault updates

* vault agent updates

* remove vault configs

* revert to vautl client

* configure vault certs

* finish updates

* revert values

* revert values

* revert values

---------

Co-authored-by: Luna Xu <[email protected]>

* customize namespace (#352)

* Update role-service for gitops (#356)

* pass in storage service to role service

* remove duplicate

* add tenant crd (#351)

* add event watch (#396)

* add csmtenants access to proxy-server (#403)

* add csmtenants access to proxy-server

* add csmtenants access to proxy-server

* remove storage service (#411)

* Use default openshift ingress (#414)

* use default openshift ingress

* update comments

* update crds for storage, role, and tenant (#415)

* [KRV-21812] Storage capacity poll interval (#416)

* [KRV-21812] Added storagePollInterval param

* [KRV-21812] Rename param

* [KRV-21812] Move param in config map

* [KRV-21812] Comment

* [KRV-21812] Capitalize parameter

* Sreekb/krv 17923 gitops (#419)

Helm chart update to deploy Redis with sentinels.

* add vault role to values (#422)

* Add snapshot policy and storage service compatibility (#423)

* Add snapshots create policy

* Add clusterroles for storage service

* Add leaderelection arg

* Address PR comments

* Address PR comments

* chart/csm-authorization support authorization-controller deployment in cluster (#429)

* add support for authorization-controller deployment in cluster

* add support for authorization-controller deployment in cluster

* add password to redis commander (#430)

* fix rebase

* address PR comments

---------

Co-authored-by: Luna Xu <[email protected]>
Co-authored-by: shaynafinocchiaro <[email protected]>
Co-authored-by: alikdell <[email protected]>
Co-authored-by: EvgenyUglov <[email protected]>
Co-authored-by: Bharath Sreekanth <[email protected]>
Co-authored-by: Fernando Alfaro Campos <[email protected]>

* Updated SDC Version (#460)

* make roundoff_file_system param to true (#462)

* make roundoff_file_system param default to true (#463)

* [charts/csm-authorization-v2.0] Move Authorization V2 to a new chart (#464)

* move authorization v2 to separate helm chart

* update v2 version in chart.yaml

* add new line

* remove storage secret

* Updated migrator and node-rescanner versions for 1.11 (#467)

* [charts/csm-application-mobility] Fix am charts (#466)

* Change 0.4 -> 0.3, there is no 0.4 AM version

* Update values.yaml

---------

Co-authored-by: shaynafinocchiaro <[email protected]>

* Merge main branch into release-v1.11 branch (#468)

* Release 1.10.1 (#401)

* Fix Helm deployment of vxflexos driver (#397)

* [csm-application-mobility] Revert app mobility version changes (#399)

* Revert app mobility version changes

---------

Co-authored-by: KshitijaKakde <[email protected]>
Co-authored-by: gallacher <[email protected]>

* Update driver/module versions for 10.0.2 (#424)

* Revert "Update driver/module versions for 10.0.2 (#424)" (#425)

* Release v1.10.2 rebase resolved (#440)

* Update driver/module versions for  10.0.2 (#426)

* removed extra space from sidecar suffix (#433)

* Update cosi versions (#434)

* Rebase branch (#436)

* Update driver/module versions for 10.0.2 (#424)

* Revert "Update driver/module versions for 10.0.2 (#424)" (#425)

---------

Co-authored-by: boyamurthy <[email protected]>

* Update driver/module versions for  10.0.2 (#426)

* removed extra space from sidecar suffix (#433)

* Update cosi versions (#434)

---------

Co-authored-by: boyamurthy <[email protected]>

* Revert "Release v1.10.2 rebase resolved (#440)" (#441)

This reverts commit 34e764e.

* Release v1.10.2 (#445)

* Update driver/module versions for  10.0.2 (#426)

* removed extra space from sidecar suffix (#433)

* Update cosi versions (#434)

* Rebase branch (#436)

* Update driver/module versions for 10.0.2 (#424)

* Revert "Update driver/module versions for 10.0.2 (#424)" (#425)

---------

Co-authored-by: boyamurthy <[email protected]>

* Update driver/module versions for  10.0.2 (#426)

* removed extra space from sidecar suffix (#433)

* Update cosi versions (#434)

* Reverse proxy issue

* Reverse proxy issue (#443)

---------

Co-authored-by: boyamurthy <[email protected]>
Co-authored-by: shefali-malhotra <[email protected]>

* Add yaml lint GitHub Action (#447)

* add yamllint github action

* add exception

* Update helm chart validation action (#449)

* remove outdated helm-validations actions and replace with updated action

* don't validate maintainers

* disable error on braces

* update rule

* update rules

* update rules

* ignore template and crd files

* lint file

* version changes

* edits

* ignore linting on crd files because they are autogenerated

* fix chart linting

* fix linting

* disable chart linting temporarily

* remove doc separator

---------

Co-authored-by: Adarsh Kumar Yadav <[email protected]>
Co-authored-by: KshitijaKakde <[email protected]>
Co-authored-by: gallacher <[email protected]>
Co-authored-by: boyamurthy <[email protected]>
Co-authored-by: shefali-malhotra <[email protected]>
Co-authored-by: HarishH-DELL <[email protected]>
Co-authored-by: Chiman Jain <[email protected]>

* Updated Encryption images for CSM 1.11 (#470)

* Fix version mismatch in 1.11 branch (#471)

* Fix version mismatch in 1.11 branch

* Revert app mobility changes

* Delete charts/csm-application-mobility directory (#472)

* Address PR comments for "Release v1.11.0" (#473)

* address PR comments

* close quotation marks

* add new line

* make sdc image consistent

* Update Health Monitor Sidecar (#474)

---------

Co-authored-by: Surya Gupta <[email protected]>
Co-authored-by: Adarsh Kumar Yadav <[email protected]>
Co-authored-by: KshitijaKakde <[email protected]>
Co-authored-by: gallacher <[email protected]>
Co-authored-by: boyamurthy <[email protected]>
Co-authored-by: bogdanNovikovDell <[email protected]>
Co-authored-by: Rishabh Raj <[email protected]>
Co-authored-by: Aaron Tye <[email protected]>
Co-authored-by: Luna Xu <[email protected]>
Co-authored-by: Luna Xu <[email protected]>
Co-authored-by: mdutka-dell <[email protected]>
Co-authored-by: coulof <[email protected]>
Co-authored-by: HarishH-DELL <[email protected]>
Co-authored-by: Waji <[email protected]>
Co-authored-by: delldubey <[email protected]>
Co-authored-by: alexemc <[email protected]>
Co-authored-by: Bharath Sreekanth <[email protected]>
Co-authored-by: JacobGros <[email protected]>
Co-authored-by: alikdell <[email protected]>
Co-authored-by: EvgenyUglov <[email protected]>
Co-authored-by: Fernando Alfaro Campos <[email protected]>
Co-authored-by: shefali-malhotra <[email protected]>
Co-authored-by: Santhosh Lakshmanan <[email protected]>
Co-authored-by: Chiman Jain <[email protected]>
Co-authored-by: Akshay Saini <[email protected]>
Co-authored-by: Trevor Dawe <[email protected]>
@anandrajak1 anandrajak1 deleted the retriever-with-encryption-fix branch October 17, 2024 19:53
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.

9 participants