Provide different running modes for node-driver-registrar, add a run mode to detect if the kubelet plugin registration failed #152
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
lgtm
| // get a callback through GetInfo, as a workaround if we don't get a callback within | ||
| // the next 10 seconds we'll restart | ||
| go func() { | ||
| err := wait.PollImmediate(100*time.Millisecond, 10*time.Second, func() (bool, error) { |
There was a problem hiding this comment.
I could adjust the timeout based on how soon we get a callback, the timing in my dev cluster was:
- linux: around 2s
- windows: around 1.7s
There was a problem hiding this comment.
In general, I think its's preferable to use a readiness probe so that the behavior and thresholds can be configurable by users.
However, http probes don't work well if the pod needs to run on host network (which most csi node plugins have to do). An exec probe is better in that case.
There was a problem hiding this comment.
good to know this, yes another idea that I posted on #143 was to use probes however I didn't know what to check on the probe, on successful registration we could touch a file that could be checked for existence with the exec probe but at the same time cleaning that file could generate more problems too, I could connect to the api-server to ask if this component is registered too but this would probably need RBAC rules and so on.
Another idea was to use a flag in the CLI for the timeout, because the state checked is only local I believe that checking every 100ms would be fine, there are two flags to control timeouts (one deprecated) and we could have another one for the kubelet registration timeout:
connectionTimeout = flag.Duration("connection-timeout", 0, "The --connection-timeout flag is deprecated")
operationTimeout = flag.Duration("timeout", time.Second, "Timeout for waiting for communication with driver")
// new flag
kubeletRegistrationTimeout = flag.Duration("kubelet-registration-timeout", 5*time.Second, "Timeout for waiting kubelet registration")
There was a problem hiding this comment.
Can the file be put into an emptydir directory? That will get cleaned up when the pod gets deleted.
There was a problem hiding this comment.
just want to understand, comparing this current implementation and "touch file" approach, what are pros and cons?
this one seems simpler?
There was a problem hiding this comment.
I think that the current implementation is simpler and self contained (no additional setup on the yaml), however as Michelle said it lacks a way to control the timeout which we could do with a cli flag too.
For the touch file approach in a experiment I deployed this component (that timed out after 10s) together the GCE PD CSI driver and if I remember correctly only this container was restarted, I'm going to deploy the version that crashed the container again and see if the pods get restarted.
There was a problem hiding this comment.
it's 3 things: timeout, polling frequency, and whether or not you even want to turn on the behavior.
There was a problem hiding this comment.
I checked that only the node-driver-registrar container is restarted:
Also the docs say:
Note: A container crashing does not remove a Pod from a node. The data in an emptyDir volume is safe across container crashes.
The touch file approach would be something like this:
func (e registrationServer) GetInfo(ctx context.Context, req *registerapi.InfoRequest) (*registerapi.PluginInfo, error) {
klog.Infof("Received GetInfo call: %+v", req)
// TODO: touch file /kubelet-registration-ack/ack
return ®isterapi.PluginInfo{
Type: registerapi.CSIPlugin,
Name: e.driverName,
}
}
func main() {
// TODO: add trap to delete the temp file
}
- name: csi-driver-registrar
image: gcr.io/mauriciopoppe-gke-dev/csi-node-driver-registrar:canary
imagePullPolicy: Always
args:
- --v=5
- --csi-address=unix://C:\\csi\\csi.sock
- --kubelet-registration-path=C:\\var\\lib\\kubelet\\plugins\\pd.csi.storage.gke.io\\csi.sock
env:
- name: KUBE_NODE_NAME
valueFrom:
fieldRef:
fieldPath: spec.nodeName
volumeMounts:
- name: kubelet-registration-ack-dir
mountPath: /kubelet-registration-ack
livenessProbe:
exec:
command:
- cat
- /kubelet-registration-ack/ack
initialDelaySeconds: 1
periodSeconds: 1
A problem that I see is if node-driver-registrar gets forcefully restarted without cleaning the temp file, the next time it comes up it could think that it's alive because the file is there but it could be possible that it didn't perform the registration process correctly.
There was a problem hiding this comment.
Can you delete the file on startup?
One more challenge is that the node-driver-registrar container doesn't have a shell. So it may need to be a command line option to do the readiness check.
There was a problem hiding this comment.
That approach sounds good, I thought about creating it with the name of the driver e.g.
args:
- --v=5
- --csi-address=unix://C:\\csi\\csi.sock
- --kubelet-registration-ack-path=C:\\registration\\pd.csi.storage.gke.io-kubelet-registration-ack
- --kubelet-registration-path=C:\\var\\lib\\kubelet\\plugins\\pd.csi.storage.gke.io\\csi.sock
livenessProbe:
exec:
command:
- /node-driver-registrar --kubelet-registration-check-ack=C:\\registration\\pd.csi.storage.gke.io-kubelet-registration-ack
initialDelaySeconds: 1
periodSeconds: 1
There would be 2 additional flags:
--kubelet-registration-ack-paththat tells this component where to write the lock file once it gets a callback from the kubelet--kubelet-registration-check-ackwith the same value as kubelet-registration-ack-path
The name of driver could be fetched from the driver itself however it makes it difficult during a forced restart when we have to delete the file on startup because we don't know which file to delete until the gRPC request to driver is made, that's why I opted for a 2 flag approach.
|
/lgtm |
|
@lizhuqi: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| @@ -63,6 +65,19 @@ func nodeRegister(csiDriverName, httpEndpoint string) { | |||
| // Registers kubelet plugin watcher api. | |||
| registerapi.RegisterRegistrationServer(grpcServer, registrar) | |||
|
|
|||
| // Sometimes on windows after registration with the kubelet plugin we don't | |||
There was a problem hiding this comment.
Can you open up a separate bug to investigate why we don't get the callback sometimes?
| // get a callback through GetInfo, as a workaround if we don't get a callback within | ||
| // the next 10 seconds we'll restart | ||
| go func() { | ||
| err := wait.PollImmediate(100*time.Millisecond, 10*time.Second, func() (bool, error) { |
There was a problem hiding this comment.
In general, I think its's preferable to use a readiness probe so that the behavior and thresholds can be configurable by users.
However, http probes don't work well if the pod needs to run on host network (which most csi node plugins have to do). An exec probe is better in that case.
Usage is documented in kubernetes-csi#152
k8s-ci-robot
removed
the
lgtm
|
Without any flags i.e. as it is right now: With the liveness probe and without the file creation flag in the linux container, in this case because the file is never created then the probe will always fail containers:
- name: csi-driver-registrar
image: gcr.io/mauriciopoppe-gke-dev/csi-node-driver-registrar:canary
imagePullPolicy: Always
args:
- "--v=5"
- "--csi-address=/csi/csi.sock"
- "--kubelet-registration-path=/var/lib/kubelet/plugins/pd.csi.storage.gke.io/csi.sock"
volumeMounts:
- name: kubelet-registration-ack
mountPath: /registration-ack
livenessProbe:
exec:
command:
- /csi-node-driver-registrar
- --kubelet-registration-check-ack=/registration-ack/pd.csi.storage.gke.io-kubelet-registration-ack
initialDelaySeconds: 2
volumes:
- name: kubelet-registration-ack
emptyDir: {}With both flags set: containers:
- name: csi-driver-registrar
image: gcr.io/mauriciopoppe-gke-dev/csi-node-driver-registrar:canary
imagePullPolicy: Always
args:
- "--v=5"
- "--csi-address=/csi/csi.sock"
- "--kubelet-registration-ack-path=/registration-ack/pd.csi.storage.gke.io-kubelet-registration-ack"
- "--kubelet-registration-path=/var/lib/kubelet/plugins/pd.csi.storage.gke.io/csi.sock"
volumeMounts:
- name: kubelet-registration-ack
mountPath: /registration-ack
livenessProbe:
exec:
command:
- /csi-node-driver-registrar
- --kubelet-registration-check-ack=/registration-ack/pd.csi.storage.gke.io-kubelet-registration-ack
initialDelaySeconds: 2
volumes:
- name: kubelet-registration-ack
emptyDir: {} |
Usage is documented in kubernetes-csi#152
mauriciopoppe
force-pushed
the
restart-if-not-connected
branch
from
dda7dcd to
79f0aac
Compare
k8s-ci-robot
added
size/L
|
|
||
| // kubelet registration succeed flags | ||
| kubeletRegistrationAckPath = flag.String("kubelet-registration-ack-path", "", "If set, a temp file with this name will be created after the kubelet registration process succeeds.") | ||
| kubeletRegistrationCheckAck = flag.String("kubelet-registration-check-ack", "", "Checks that the kubelet plugin registration ack file exists, if set it must be the same value as kubelet-registration-ack-path.") |
There was a problem hiding this comment.
could only use one value kubeletRegistrationAckPath, if kubeletRegistrationAckPath is set, then always check where kubelet plugin registration succeeds or not, that would be simpler to configure.
There was a problem hiding this comment.
yes, that was my initial take but I found some problems with the action to perform, so I'm checking whether a lock file exists or not (per driver) and to do that this flag would need to have a value so the probe could be:
livenessProbe:
exec:
command:
- /csi-node-driver-registrar
- "--kubelet-registration-ack-path=my-driver-lock-file"
initialDelaySeconds: 2
The container needs to create this file on startup:
containers:
- name: csi-driver-registrar
image: gcr.io/mauriciopoppe-gke-dev/csi-node-driver-registrar:canary
args:
- "--kubelet-registration-ack-path=my-driver-lock-file
- "--kubelet-registration-path=/var/lib/kubelet/plugins/pd.csi.storage.gke.io/csi.sock"
The logic to use it would be:
- if it's the only flag set then we'll use it as a probe
- if it's together with
--kubelet-registration-paththen it's not a probe and we should use it to create the file
The fact that there are two behaviors for the same flag felt kinda weird but I could try this approach too
There was a problem hiding this comment.
I've updated the flags usage, now there's one flag to set the lock file path and another one to set the mode:
containers:
- name: csi-driver-registrar
image: gcr.io/mauriciopoppe-gke-dev/csi-node-driver-registrar:canary
args:
- "--kubelet-registration-succeeded-lock-file=my-driver-lock-file"
- "--kubelet-registration-path=/var/lib/kubelet/plugins/pd.csi.storage.gke.io/csi.sock"
livenessProbe:
exec:
command:
- /csi-node-driver-registrar
- "--kubelet-registration-succeeded-lock-file=my-driver-lock-file"
- "--kubelet-registration-succeeded-mode=probe"
initialDelaySeconds: 2
The logic to use them is:
- to enable checking the registration process set
--kubelet-registration-succeeded-lock-fileto the path of a file that will be created in an emptydir, this value must be set in both the image args and the liveness probe exec binary args with the same value - in addition set
--kubelet-registration-succeeded-mode=probein the liveness probe
There was a problem hiding this comment.
Do we need the lock file arg? Can we have a predefined file name based on the driver name?
There was a problem hiding this comment.
the driver name is available a few lines after the fix and it's fetched through an RPC call, I could go through this path once and cache it in memory for later calls of the probe
Usage is documented in kubernetes-csi#152
mauriciopoppe
force-pushed
the
restart-if-not-connected
branch
from
79f0aac to
d72b499
Compare
mauriciopoppe
force-pushed
the
restart-if-not-connected
branch
3 times, most recently
from
b64889e to
4c0a68c
Compare
mauriciopoppe
changed the title
mauriciopoppe
force-pushed
the
restart-if-not-connected
branch
from
4c0a68c to
58bd237
Compare
|
Thanks for the feedback @msau42, now the registration file is always created when the kubelet plugin registration succeeds, however, doing a health check with a probe is optional. I've tested the new implementation in Windows nodes: These are the logs when node-driver-registrar is started without the code that creates the registration file (I commented out the code to test it): Kubelet is restarting the component as expected:
|
| fmt.Println(os.Args[0], version) | ||
| return | ||
| // set after we made sure that *kubeletRegistrationPath exists | ||
| kubeletRegistrationPathDir := filepath.Dir(*kubeletRegistrationPath) |
There was a problem hiding this comment.
Do you need the path to the file? Should the argument be the path to the directory?
There was a problem hiding this comment.
I think that using different values for the same arg might be a bit confusing and that's why the docs ask the user to use the same value as the one used in the container args, I thought it'd be easier to just copy & paste the same value but you're right that we actually need the path to the directory (in the implementation this is computed with filepath.Dir).
We could use the same arg with the path to the directory as the value with additional documentation too.
There was a problem hiding this comment.
Oh I forgot that we're reusing the same argument. Then I think this is fine.
| if modeIsKubeletRegistrationProbe() { | ||
| lockfileExists, err := util.DoesFileExist(registrationProbePath) | ||
| if err != nil { | ||
| fmt.Printf("Failed to check if registration path exists, registrationProbePath=%s err=%v", registrationProbePath, err) |
There was a problem hiding this comment.
klog.Fatalf should work here. Same below.
There was a problem hiding this comment.
When I used klog it'd print the golang debug line in the pod events, something like:
liveness probe failed: EDDMM date main.go:###] Failed to ...
I thought that just by using fmt.Printf I could get only the message e.g.
liveness probe failed: Failed to ...
There was a problem hiding this comment.
I think it's good to have consistency with the other logs we do. Having timestamp information is useful for debugging.
|
/lgtm Thanks! |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andyzhangx, mauriciopoppe, msau42 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What type of PR is this?
/kind bug
What this PR does / why we need it:
Adds running modes, there's a new kubelet-plugin-exec running mode that checks if the kubelet plugin registration succeeded, please check the README.md for an example of how it's used.
Which issue(s) this PR fixes:
Workaround for #143, unfortunately it isn't fixed yet.
Does this PR introduce a user-facing change?:
/cc @jingxu97 @msau42
cc @lizhuqi @andyzhangx