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

feat(probe): add size change detection #616

Merged
merged 20 commits into from
Sep 6, 2021

Conversation

z0marlin
Copy link
Contributor

@z0marlin z0marlin commented Aug 18, 2021

Add blockdevice size change detection using udev change events

Checklist:

@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2021

Codecov Report

Merging #616 (b5778d7) into master (816bf0c) will increase coverage by 0.54%.
The diff coverage is 12.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #616      +/-   ##
==========================================
+ Coverage   46.38%   46.93%   +0.54%     
==========================================
  Files          78       78              
  Lines        3781     3814      +33     
==========================================
+ Hits         1754     1790      +36     
+ Misses       1871     1866       -5     
- Partials      156      158       +2     
Impacted Files Coverage Δ
cmd/ndm_daemonset/probe/changehandler.go 0.00% <0.00%> (ø)
cmd/ndm_daemonset/probe/eventhandler.go 38.88% <0.00%> (+8.33%) ⬆️
cmd/ndm_daemonset/probe/mountprobe.go 19.58% <0.00%> (-0.63%) ⬇️
cmd/ndm_daemonset/probe/sysfsprobe.go 0.00% <0.00%> (ø)
pkg/features/features.go 100.00% <ø> (ø)
cmd/ndm_daemonset/probe/udevprobe.go 50.19% <21.05%> (+1.66%) ⬆️
pkg/epoll/epoll.go 62.50% <42.85%> (-1.98%) ⬇️
cmd/ndm_daemonset/probe/addhandler.go 77.47% <0.00%> (+6.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 816bf0c...b5778d7. Read the comment docs.

@@ -214,3 +214,21 @@ func (disk *Disk) Unmount() error {
}
return lastErr
}

func (disk *Disk) Resize(size int64) error {
units := [4]string{"", "K", "M", "G"}
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be something in this package to help you with this. https://pkg.go.dev/k8s.io/kubernetes@v1.22.0/pkg/api/v1/resource

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RealHarshThakur I wasn't able to figure out how to use the package here. Could you please help out?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Plan is to switch API fields to use this type too but we need to upgrade the CRD version.

@akhilerm akhilerm self-requested a review August 19, 2021 04:42
"k8s.io/klog"
)

func (pe *ProbeEvent) changeBlockDevice(bd *blockdevice.BlockDevice, requestedProbes ...string) error {
bdCopy := *bd
equalMountPoints := true
Copy link
Contributor

Choose a reason for hiding this comment

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

A better variable name can be chosen. This is a bit confusing.

@@ -34,7 +34,8 @@ const (
// DetachEA is detach disk event name
DetachEA EventAction = libudevwrapper.UDEV_ACTION_REMOVE
// MountEA is mount-point/fs change event
MountEA EventAction = "mount-change"
MountEA EventAction = "mount-change"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we need 2 separate events now?

Comment on lines 122 to 123
if features.FeatureGates.IsEnabled(features.SizeChangeDetection) ||
features.FeatureGates.IsEnabled(features.MountChangeDetection) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this into one feature flag, instead of having 2 separate features?

eventMessage.RequestedProbes = []string{udevProbeName,
sysfsProbeName}
} else {
eventMessage.RequestedProbes = []string{udevProbeName}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is for detecting the filesystem changes right? This also need to be done only if the feature gate is enabled.


// SizeChangeDetection feature flag is used to enable size
// change detection for block devices
SizeChangeDetection Feature = "SizeChangeDetection"
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using 2 separate feature gates, we should use just one feature gate as both of them are similar

Copy link
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

@z0marlin some questions and changes are needed.

@z0marlin z0marlin force-pushed the sizech-udev branch 2 times, most recently from 79c1975 to 60cd0d1 Compare August 20, 2021 13:07
MountChangeDetection = "MountChangeDetection"
// ChangeDetection is used to enable detecting changes to
// blockdevice size, filesystem, and mount-points.
ChangeDetection Feature = "ChangeDetection"
Copy link
Contributor

Choose a reason for hiding this comment

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

This change also need to be made in the operator and chart files in the deploy/ directory

Copy link
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

@z0marlin have a few nitpick comments

deploy/ndm-operator.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

a few nitpick comments on the operator yaml

akhilerm
akhilerm previously approved these changes Aug 22, 2021
Copy link
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

LGTM

akhilerm
akhilerm previously approved these changes Aug 23, 2021
Copy link
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

LGTM

@kmova kmova added this to Pre-commits and Designs - Due: Aug 30 2021 in 3.0 Release Tracker - Released Sept 24th. Aug 24, 2021
@kmova kmova added this to Under development in NDM Aug 29, 2021
akhilerm
akhilerm previously approved these changes Aug 30, 2021
Copy link
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @z0marlin

@akhilerm akhilerm added the pr/hold-merge The PR should not be merged now label Aug 30, 2021
z0marlin and others added 14 commits September 6, 2021 11:07
Signed-off-by: Aditya Jain <aditya.jainadityajain.jain@gmail.com>
process all udev events, including change events in udev probe alone
instead of processing them in other probes. The udev probe alone can
send out required event message after processing the change events.
Since the remaining two types - add and remove are also handled by udev
probe, keeping the third type (change) too in the same probe is cleaner
and logical.

Signed-off-by: Aditya Jain <aditya.jainadityajain.jain@gmail.com>
the change handling mechanism is currently used to detect changes
in blockdevice size, filesystem, and mount-points. Add checks to prevent
calls to the kube api server in case any of these fields haven't changed
after re-filling the blockdevice details using the probes.

Signed-off-by: Aditya Jain <aditya.jainadityajain.jain@gmail.com>
Signed-off-by: Aditya Jain <aditya.jainadityajain.jain@gmail.com>
merge the two flags into a single flag - `ChangeDetection`

Signed-off-by: Aditya Jain <aditya.jainadityajain.jain@gmail.com>
Signed-off-by: Aditya Jain <aditya.jainadityajain.jain@gmail.com>
Signed-off-by: Aditya Jain <aditya.jainadityajain.jain@gmail.com>
Co-authored-by: Akhil Mohan <akhilerm@gmail.com>
Signed-off-by: Aditya Jain <aditya.jainadityajain.jain@gmail.com>
Signed-off-by: Aditya Jain <aditya.jainadityajain.jain@gmail.com>
Signed-off-by: Aditya Jain <aditya.jainadityajain.jain@gmail.com>
Signed-off-by: Aditya Jain <aditya.jainadityajain.jain@gmail.com>
Signed-off-by: Aditya Jain <aditya.jainadityajain.jain@gmail.com>
Signed-off-by: Aditya Jain <aditya.jainadityajain.jain@gmail.com>
Signed-off-by: Aditya Jain <aditya.jainadityajain.jain@gmail.com>
z0marlin and others added 3 commits September 6, 2021 11:44
Signed-off-by: Aditya Jain <aditya.jainadityajain.jain@gmail.com>
Signed-off-by: Akhil Mohan <akhil.mohan@mayadata.io>
Signed-off-by: Aditya Jain <aditya.jainadityajain.jain@gmail.com>
@z0marlin
Copy link
Contributor Author

z0marlin commented Sep 6, 2021

Added fixes for #625

Copy link
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

LGTM

@akhilerm akhilerm removed the pr/hold-merge The PR should not be merged now label Sep 6, 2021
@akhilerm akhilerm merged commit eb1aa4b into openebs-archive:master Sep 6, 2021
NDM automation moved this from Under development to Done Sep 6, 2021
3.0 Release Tracker - Released Sept 24th. automation moved this from Pre-commits and Designs - Due: Aug 30 2021 to Done Sep 6, 2021
@z0marlin z0marlin deleted the sizech-udev branch October 12, 2021 05:18
@Abhinandan-Purkait Abhinandan-Purkait removed this from Done in NDM Apr 29, 2024
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.

Add support for updating mountpoint and capacity of blockdevices without NDM pod restart
4 participants