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.

Sign up for GitHub

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

Jump to bottom

fix: Correct the CSI handler logic #603

Merged
merged 10 commits into from
May 8, 2024
Merged

fix: Correct the CSI handler logic #603

merged 10 commits into from
May 8, 2024

Conversation

dlipovetsky
Copy link
Contributor

@dlipovetsky dlipovetsky commented May 1, 2024 •
edited
Loading

What problem does this PR solve?:

  • Return an error when the CSI variable is defined, but the
    Providers list is empty.
  • Return an error when the CSI provider is defined, but its
    StorageClassConfig list is empty.
  • Return an error when any CSI provider is unknown.
  • Return an error when DefaultStorage is empty.
  • Return an error when the DefaultStorage Provider Name does not match a set provider.
  • Return an error when the DefaultStorage StorageClassConfig Name does not match the name of a StorageClassConfig for the default provider.

Adds unit tests to verify this logic, and more. Covers the majority of the generic CSI handler AfterControlPlaneInitialized hook.

Which issue(s) this PR fixes:
Fixes #

How Has This Been Tested?:

Adds a unit test; I think this is the first unit test in CAREN to exercise a handler request-response flow.

Special notes for your reviewer:

I noticed these issues as I was studying the CSI handler in order to write the ServiceLoadbalancer handler. I'll be adding a unit test to #592 as well.

@dlipovetsky dlipovetsky requested review from faiq and jimmidyson May 1, 2024 23:20
@github-actions github-actions bot added the fix label May 1, 2024
@jimmidyson
Copy link
Member

Pushed a PR targeting your PR branch 😅 dlipovetsky#1

@dlipovetsky
Copy link
Contributor Author

Pushed a PR targeting your PR branch 😅 dlipovetsky#1

Thank you. Merged!

dkoshkin and others added 4 commits May 3, 2024 09:18
@dkoshkin @dlipovetsky
feat: Add utility functions for un/marshalling variables
ec2473c
@dlipovetsky
fix: Correct the CSI handler logic
1dd6515 
- Return an error when the CSI variable is defined, but the
  Providers list is empty.
- Return an error when the CSI provider is defined, but its
  StorageClassConfig list is empty.
- Return an error when there two or more CSI providers are defined,
  but the DefaultStorageConfig is empty.
- Return an error when any CSI provider is unknown.

Adds a unit test to verify this logic.
@jimmidyson @dlipovetsky
fix: Add API validation for CSI providers and storageclasses
529cd06 
This ensures that users specify the requried details via API validation
rather than when deploying CSI, provividing a better UX with faster
feedback.
@dlipovetsky
fixup! fix: Correct the CSI handler logic
ebb4f78 
- Makes CSI.DefaultStorage required
- Return an error if the DefaultStorage ProviderName does not match a
  set provider.
- Return an error if the DefaultStorage StorageClassConfigName does not
  match the name of a StorageClassConfig of the default provider.
@dlipovetsky
Copy link
Contributor Author

I rebased on main and force-pushed. The conflict was with kubebuilder tags, nothing related to these changes.

I added changes in a new commit, so previous reviews are still valid.

I added stricter validation for the default storage configuration as well.

The unit test covers all of the handler code, with the exception of two error cases, one for failing to read the CSI variable, and another for failing to apply the driver. I'll see about handling these as well.

@dlipovetsky
Copy link
Contributor Author

I added a test to confirm we return an error on handler failure. I'm not going to add a test for failing to read the CSI variable for now.

@github-actions github-actions bot added fix and removed fix labels May 3, 2024
dkoshkin
dkoshkin reviewed May 3, 2024
View reviewed changes
Copy link
Contributor

@dkoshkin dkoshkin left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, the unit tests are really helpful to understand these complicated API interactions.

pkg/handlers/generic/lifecycle/csi/handler.go Outdated Show resolved Hide resolved
pkg/handlers/generic/lifecycle/csi/handler.go Outdated Show resolved Hide resolved
dkoshkin
dkoshkin reviewed May 3, 2024
View reviewed changes
api/variables/util.go Outdated Show resolved Hide resolved
dlipovetsky added 2 commits May 6, 2024 10:27
@dlipovetsky
fixup! fix: Correct the CSI handler logic
043d4bd 
Move utility functions
@dlipovetsky
fixup! fix: Correct the CSI handler logic
e535246 
Require StorageClassConfig only if it's referenced by DefaultStorage
@dlipovetsky
fixup! fix: Correct the CSI handler logic
c4fa674 
When needed, explicitly pass nil to log.Error calls
@dlipovetsky
Copy link
Contributor Author

@faiq noticed some misleading log.Error calls. Thanks for catching that! Fixed in c4fa674

@dlipovetsky
fixup! fix: Correct the CSI handler logic
e0f134d 
Factor DefaultStorage validation to helper function
faiq
faiq approved these changes May 7, 2024
View reviewed changes
Copy link
Contributor

@faiq faiq left a comment

Choose a reason for hiding this comment

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

looks great. thanks for taking in the feedback

@jimmidyson jimmidyson merged commit f091343 into nutanix-cloud-native:main May 8, 2024
16 checks passed
@github-actions github-actions bot mentioned this pull request May 8, 2024
Merged
jimmidyson added a commit that referenced this pull request May 21, 2024
@jimmidyson @github-actions
release(main): v0.9.0 (#597)
0ce44c2 
🤖 I have created a release *beep* *boop*
---


## 0.9.0 (2024-05-21)

<!-- Release notes generated using configuration in .github/release.yaml
at main -->

## What's Changed
### Exciting New Features 🎉
* feat: expose GenerateNoProxy func by @mhrabovcin in
#594
* feat: Add the ServiceLoadbalancer Addon, with MetalLB as first
provider by @dlipovetsky in
#592
* feat: adds GPU mutation by @faiq in
#591
* feat: Add GenericClusterConfig and add docs on usage with own CC by
@jimmidyson in
#606
* feat: Enable unprivileged ports sysctl in containerd config by
@jimmidyson in
#645
* feat: API for encryption at-rest by @supershal in
#610
* feat: Bump sigs.k8s.io/cluster-api to v1.7.2 by @jimmidyson in
#661
* feat: Pull calico images from quay.io instead of docker hub by
@jimmidyson in
#676
* feat: update cluster autoscaler to v1.30.0 by @dkoshkin in
#681
### Fixes 🔧
* fix: Fix error messages returned by HelmChartGetter by @dlipovetsky in
#598
* fix: use a consistent MachineDeployment class name by @dkoshkin in
#612
* fix: Do not return error if serviceLoadBalancer field is not set by
@dlipovetsky in
#611
* fix: use provided options for serverside apply by @supershal in
#627
* fix: Correct the CSI handler logic by @dlipovetsky in
#603
* fix: Fix the internal ClusterConfig type used for provider-agnostic
logic by @jimmidyson in
#607
* fix: log mutation failure errors by @supershal in
#649
* fix: Always apply containerd patches by @jimmidyson in
#644
* fix: cluster-autoscaler Helm values for workload clusters by @dkoshkin
in
#658
* fix: Make Cluster the owner of image registry credential secret by
@dlipovetsky in
#648
* fix: Upgrade dynamic-credential-provider to v0.5.3 by @jimmidyson in
#677
### Other Changes
* build: Add v0.8 release metadata by @jimmidyson in
#595
* refactor: Clean up API constants, and explain usage by @dlipovetsky in
#588
* docs: Add how to deploy CAREN by @jimmidyson in
#599
* docs: Upgrade hugo to latest by @jimmidyson in
#601
* docs: Update addons docs and tweak release doc by @jimmidyson in
#596
* build: Ensure provider metadata is up to date when releasing by
@jimmidyson in
#600
* docs: Add how to create clusters by @jimmidyson in
#602
* docs: Update docsy module by @jimmidyson in
#605
* refactor: Apply kubebuilder annotations for required/optional
everywhere by @jimmidyson in
#604
* docs: Cluster Autoscaler is deployed on the management cluster by
@dkoshkin in
#608
* docs: Fix missing placeholder in "create nutanix cluster" doc by
@dlipovetsky in
#609
* refactor: Remove unused api/variables package by @dlipovetsky in
#623
* refactor: move label helper functions to utils package by @supershal
in
#626
* build: Use go1.22.3 toolchain to mitigate vulnerabilties by
@jimmidyson in
#628
* build: Temporary lint config fix until next golangci-lint release by
@jimmidyson in
#629
* build: Update license for Nutanix by @jimmidyson in
#456
* test(e2e): Consistent core/bootstrap/control-plane provider versions
by @jimmidyson in
#639
* ci: free up disk space before running tests by @dkoshkin in
#643
* test: Add more context to panic in envtest helper by @dlipovetsky in
#641
* refactor: Use colon to separate context from wrapped error by
@dlipovetsky in
#642
* refactor: Remove unused test helper function by @dlipovetsky in
#647
* test: Add even more context to panic in envtest helper by @dlipovetsky
in
#650
* build: Make module-relative "go list -m" compatible with GOWORK by
@dlipovetsky in
#651
* test: Match cluster namespace to cluster name by @dlipovetsky in
#652
* refactor: Write configuration under /etc/caren by @dlipovetsky in
#656
* build: use a shorter namespace caren-system by @dkoshkin in
#662
* refactor: Use a Credentials struct consistently by @dlipovetsky in
#663
* test: add encryptionAtRest config in capi-quick-start by @supershal in
#659
* test(e2e): Fix up secret ownership checks by @jimmidyson in
#665
* test: Remove hard-coded text focus and label for e2e tests by
@dlipovetsky in
#667
* ci: Use new dependabot multimodule capabilities by @jimmidyson in
#664
* refactor: aggregate types to be used by clients by @dkoshkin in
#672
* test: Add E2E_DRYRUN and E2E_VERBOSE make vars by @dlipovetsky in
#666
* build: Ignore all gitlint rules for dependabot commits by @jimmidyson
in
#675
* build: Update all tools by @jimmidyson in
#678
* test(e2e): Use upstream CRS helpers by @jimmidyson in
#680
* build: Correct dry-run output by @jimmidyson in
#679
* build: Use k8s v1.29.4 as default Kubernetes version by @jimmidyson in
#646

## New Contributors
* @prajnutanix made their first contribution in
#638

**Full Changelog**:
v0.8.1...v0.9.0

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkoshkin dkoshkin dkoshkin approved these changes

@jimmidyson jimmidyson jimmidyson left review comments

@faiq faiq faiq approved these changes

Assignees
No one assigned
Labels
fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dlipovetsky @jimmidyson @dkoshkin @faiq