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

Added support for CSI-PowerScale #119

Merged
merged 4 commits into from
May 11, 2022
Merged

Conversation

Sakshi-dell
Copy link
Contributor

@Sakshi-dell Sakshi-dell commented Apr 27, 2022

Description

A few sentences describing the overall goals of the pull request's commits.

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #
dell/csm#262

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

  • Created podmon image and used the same to install CSI-PowerScale driver
  • Ran existing unit-tests successfully

Unit-test Output
PASS
coverage: 92.6% of statements
status 0
ok podmon/internal/monitor 8.603s coverage: 92.6% of statements

randeepdell
randeepdell previously approved these changes Apr 27, 2022
cmd/podmon/main.go Outdated Show resolved Hide resolved
internal/monitor/driver.go Outdated Show resolved Hide resolved
log.Infof("CSI Driver for Unity")
monitor.Driver = new(monitor.UnityDriver)
} else if strings.Contains(*args.driverPath, "isilon") { // added condition to create instance of PowerScale driver
case strings.Contains(*args.driverPath, "isilon"):
// added condition to create instance of PowerScale driver
Copy link
Contributor

Choose a reason for hiding this comment

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

don't really need this

randeepdell
randeepdell previously approved these changes May 2, 2022
rbo54
rbo54 previously approved these changes May 2, 2022
@prablr79
Copy link

prablr79 commented May 4, 2022

@Sakshi-dell can you close the comments from Florian and we can merge this PR.

meggm
meggm previously approved these changes May 4, 2022
@prablr79
Copy link

prablr79 commented May 4, 2022

@alexemc @gallacher @sharmilarama need codeowner approval to merge this PR. Please approve.


// FinalCleanup handles any driver specific final cleanup.
func (d *PScaleDriver) FinalCleanup(rawBlock bool, volumeHandle, pvName, podUUID string) error {
return nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any corner cases where podmon needed to cleanup. If there anything we should add here.

Copy link
Collaborator

@alikdell alikdell left a comment

Choose a reason for hiding this comment

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

Please add automated unit test for code coverage.

@Sakshi-dell Sakshi-dell dismissed stale reviews from meggm, rbo54, and randeepdell via 4f1918c May 5, 2022 11:51
Copy link
Collaborator

@rbo54 rbo54 left a comment

Choose a reason for hiding this comment

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

What you have so far looks pretty good. I left a couple of comments to think about.
I am concerned about lack of staging directory in driver.go.
I think you probably will want to add additional tests.
Tom


// GetStagingMountDir Returns the staging directory used by NodeUnstage for a mount device.
func (d *PScaleDriver) GetStagingMountDir(volumeHandle, pvName string) string {
return ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there not a separate staging directory? What do you expect to be passed as the staging directory to node unstage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PowerScale does not have a separate staging path, we have not implemented NodeStage/NodeUnstage Volume.

@randeepdell randeepdell merged commit 5d23a8f into main May 11, 2022
@randeepdell randeepdell deleted the Resiliency-changes-for-powerscale branch May 11, 2022 10:06
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.

7 participants