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

Disabling SDC dependency for NFS Volume Provisioning #330

Merged
merged 26 commits into from
Oct 1, 2024

Conversation

harshitap26
Copy link
Contributor

@harshitap26 harshitap26 commented Sep 16, 2024

Description

The below changes were made a part of this PR.

  • NodeGetInfo changes to make NFS independent of SDC
  • Refactor NodeProbe to probe arrays when SDC is not deployed
  • PR to get the IPs corresponding to the Interfaces and use the IPs for NFS Volume Provisioning removing the dependency on SDC.

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #
dell/csm#663

Checklist:

  • I have performed a self-review of my own code to ensure there are no formatting, vetting, linting, or security issues
  • I have verified that new and existing unit tests pass locally with my changes
  • I have not allowed coverage numbers to degenerate
  • I have maintained at least 90% code coverage
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • Backward compatibility is not broken

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

  • Config Map is getting updated with the Interface IPs.
  • Unit tests and the scenarios are passing.
  • Cert-csi VIO for NFS and XFS (block) with SDC installed
  • Cert-csi VIO for NFS only with NO SDC installed

UT code coverage:
coverage: 90.0% of statements

Attachments: cert-csi logs for the above 2 cases.

cert-csi-NFS-sdc-installed.txt
cert-csi-NFS-sdc-NOT-installed.txt
cert-csi-XFS-sdc-installed.txt

In addition to above, also ran the following on clusters with SDC installed and not installed:

  1. cert-csi volume expansion
  2. cert-csi clone suite
  3. cert-csi multi-attach-vol

@harshitap26 harshitap26 force-pushed the usr/Harshita_Pandey/sdc-optional branch from 72751c8 to 30b74b4 Compare September 23, 2024 18:38
@harshitap26 harshitap26 marked this pull request as ready for review September 30, 2024 06:01
@harshitap26 harshitap26 changed the title Usr/harshita pandey/sdc optional Disabling SDC dependency for NFS Volume Provisioning Sep 30, 2024
go.mod Show resolved Hide resolved
service/controller.go Outdated Show resolved Hide resolved
service/controller.go Show resolved Hide resolved
service/node.go Show resolved Hide resolved
service/node.go Outdated Show resolved Hide resolved
service/service.go Outdated Show resolved Hide resolved
go 1.22
go 1.22.0

toolchain go1.22.5
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to define a toolchain here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure on this, I'd have to defer this to Harshita.

service/service.go Outdated Show resolved Hide resolved
service/service.go Outdated Show resolved Hide resolved
@tdawe
Copy link
Contributor

tdawe commented Oct 1, 2024

Branch needs to be rebased with latest from main

@falfaroc falfaroc self-requested a review October 1, 2024 18:29
Copy link
Contributor

@tdawe tdawe left a comment

Choose a reason for hiding this comment

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

Reviewed and all comments were addressed.

@bharathsreekanth bharathsreekanth merged commit 9f7b207 into main Oct 1, 2024
4 checks passed
@anandrajak1 anandrajak1 deleted the usr/Harshita_Pandey/sdc-optional branch October 17, 2024 18:42
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.

5 participants