-
Notifications
You must be signed in to change notification settings - Fork 69
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
fix(status) update cstor volume status to offline based on availability of target pods #395
Conversation
There was a problem hiding this 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
…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]>
0aaba71
to
0a25441
Compare
Signed-off-by: Rupank Pahuja <[email protected]>
There was a problem hiding this 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?
_, 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 |
There was a problem hiding this comment.
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
continue | |
return |
Signed-off-by: Rupank Pahuja <[email protected]>
Signed-off-by: Rupank Pahuja <[email protected]>
There was a problem hiding this 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
…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]>
Signed-off-by: Rupank Pahuja <[email protected]>
Signed-off-by: Rupank Pahuja <[email protected]>
dadc3ab
to
72dfae2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes are good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Fixes #377
I have tried to understand the project structure using documentation.
Also I have referred to these two PR as suggested by @mittachaitu
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