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

fix(status) update cstor volume status to offline based on availability of target pods #395

Merged

Conversation

saltperfect
Copy link
Contributor

Fixes #377
I have tried to understand the project structure using documentation.

Also I have referred to these two PR as suggested by @mittachaitu

  1. fix(status): update cvr status on graceful corresponding pool-manager termination #349
  2. fix(status): update CSPI status when corresponding cStor pool-manager terminated #141

I am yet to add testcases. This is an initial commit to get reviews and check if I have done the changes in correct file. 😸 Once I have nailed down which files, I can go ahead and improve on other aspects of the PR.

First time contributor

@saltperfect saltperfect marked this pull request as draft October 9, 2021 10:38
pkg/controllers/volume-mgmt/handler.go Outdated Show resolved Hide resolved
pkg/controllers/volume-mgmt/handler.go Outdated Show resolved Hide resolved
pkg/controllers/volume-mgmt/handler.go Outdated Show resolved Hide resolved
Copy link
Contributor

@mittachaitu mittachaitu left a comment

Choose a reason for hiding this comment

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

Provided few comments PTAL

pkg/controllers/volume-mgmt/runner.go Outdated Show resolved Hide resolved
pkg/controllers/volume-mgmt/handler.go Outdated Show resolved Hide resolved
pkg/controllers/volume-mgmt/handler.go Outdated Show resolved Hide resolved
pkg/controllers/volume-mgmt/handler.go Outdated Show resolved Hide resolved
@mittachaitu mittachaitu removed the request for review from sonasingh46 October 11, 2021 08:38
Rupank Pahuja added 2 commits October 11, 2021 22:55
…ty of target pods

Signed-off-by: Rupank Pahuja  <[email protected]>
Signed-off-by: Rupank Pahuja <[email protected]>
Signed-off-by: Rupank Pahuja  <[email protected]>
Signed-off-by: Rupank Pahuja <[email protected]>
@saltperfect saltperfect force-pushed the 377-updateCstoreVolumeStatus branch from 0aaba71 to 0a25441 Compare October 11, 2021 17:25
Copy link
Contributor

@mittachaitu mittachaitu left a comment

Choose a reason for hiding this comment

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

One more comment... Did you test the changes manually/automation way?

pkg/controllers/volume-mgmt/handler.go Show resolved Hide resolved
_, err = c.clientset.CstorV1().CStorVolumes(cv.Namespace).Update(context.TODO(), &cv, metav1.UpdateOptions{})
if err != nil {
klog.Errorf("failed to update CV: %s status to %s", cv.Name, apis.CVStatusOffline)
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

Here also, we found the CV that required to update and processing other CV's will not helpful so we can return

Suggested change
continue
return

Signed-off-by: Rupank Pahuja <[email protected]>
Copy link
Contributor

@mittachaitu mittachaitu left a comment

Choose a reason for hiding this comment

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

Looks good... Let's rollout PR from draft stage to PR

pkg/controllers/volume-mgmt/handler.go Outdated Show resolved Hide resolved
Rupank Pahuja added 7 commits October 26, 2021 22:14
…ty of target pods

Signed-off-by: Rupank Pahuja  <[email protected]>
Signed-off-by: Rupank Pahuja <[email protected]>
Signed-off-by: Rupank Pahuja  <[email protected]>
Signed-off-by: Rupank Pahuja <[email protected]>
Signed-off-by: Rupank Pahuja <[email protected]>
Signed-off-by: Rupank Pahuja <[email protected]>
Signed-off-by: Rupank Pahuja <[email protected]>
@saltperfect saltperfect force-pushed the 377-updateCstoreVolumeStatus branch from dadc3ab to 72dfae2 Compare October 26, 2021 16:54
@saltperfect saltperfect marked this pull request as ready for review October 26, 2021 16:58
@github-actions github-actions bot requested a review from sonasingh46 October 26, 2021 16:58
Copy link
Contributor

@mittachaitu mittachaitu left a comment

Choose a reason for hiding this comment

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

Changes are good

Copy link
Contributor

@prateekpandey14 prateekpandey14 left a comment

Choose a reason for hiding this comment

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

lgtm

@prateekpandey14 prateekpandey14 merged commit 6024941 into openebs-archive:develop Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update cstorVolume status based on the availablity of target pod
4 participants