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.

Sign up for GitHub

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

Jump to bottom

Replace dbus-send for fake PrepareForShutdown message #107819

Merged
merged 1 commit into from
May 4, 2022
Merged

Replace dbus-send for fake PrepareForShutdown message #107819

merged 1 commit into from
May 4, 2022

Conversation

matthyx
Copy link
Contributor

@matthyx matthyx commented Jan 27, 2022 •
edited
Loading

What type of PR is this?

/kind feature
/kind cleanup

What this PR does / why we need it:

Currently graceful node shutdown tests are skipped on Fedora because the distribution is missing dbus-send command used to send a fake signal to the bus and trigger a kubelet shutdown.

Which issue(s) this PR fixes:

Fixes #107505

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

https://pkg.go.dev/github.com/godbus/dbus#Conn.Emit

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/test sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 27, 2022
@matthyx
Copy link
Contributor Author

matthyx commented Jan 27, 2022

/test ?

@k8s-ci-robot
Copy link
Contributor

@matthyx: The following commands are available to trigger required jobs:

  • /test pull-kubernetes-conformance-kind-ga-only-parallel
  • /test pull-kubernetes-dependencies
  • /test pull-kubernetes-dependencies-go-canary
  • /test pull-kubernetes-e2e-gce
  • /test pull-kubernetes-e2e-gce-100-performance
  • /test pull-kubernetes-e2e-gce-big-performance
  • /test pull-kubernetes-e2e-gce-canary
  • /test pull-kubernetes-e2e-gce-large-performance
  • /test pull-kubernetes-e2e-gce-network-proxy-http-connect
  • /test pull-kubernetes-e2e-gce-no-stage
  • /test pull-kubernetes-e2e-gce-ubuntu
  • /test pull-kubernetes-e2e-gce-ubuntu-containerd
  • /test pull-kubernetes-e2e-gce-ubuntu-containerd-canary
  • /test pull-kubernetes-e2e-kind
  • /test pull-kubernetes-e2e-kind-ipv6
  • /test pull-kubernetes-files-remake
  • /test pull-kubernetes-integration
  • /test pull-kubernetes-integration-go-canary
  • /test pull-kubernetes-kubemark-e2e-gce-scale
  • /test pull-kubernetes-node-e2e-containerd
  • /test pull-kubernetes-typecheck
  • /test pull-kubernetes-unit
  • /test pull-kubernetes-unit-go-canary
  • /test pull-kubernetes-update
  • /test pull-kubernetes-verify
  • /test pull-kubernetes-verify-go-canary
  • /test pull-kubernetes-verify-govet-levee

The following commands are available to trigger optional jobs:

  • /test check-dependency-stats
  • /test pull-kubernetes-conformance-image-test
  • /test pull-kubernetes-conformance-kind-ga-only
  • /test pull-kubernetes-conformance-kind-ipv6-parallel
  • /test pull-kubernetes-cross
  • /test pull-kubernetes-e2e-aks-engine-azure-disk-windows-containerd
  • /test pull-kubernetes-e2e-aks-engine-azure-file-windows-containerd
  • /test pull-kubernetes-e2e-aks-engine-gpu-windows-dockershim
  • /test pull-kubernetes-e2e-aks-engine-windows-containerd
  • /test pull-kubernetes-e2e-capz-azure-disk
  • /test pull-kubernetes-e2e-capz-azure-disk-vmss
  • /test pull-kubernetes-e2e-capz-azure-file
  • /test pull-kubernetes-e2e-capz-azure-file-vmss
  • /test pull-kubernetes-e2e-capz-conformance
  • /test pull-kubernetes-e2e-capz-ha-control-plane
  • /test pull-kubernetes-e2e-containerd-gce
  • /test pull-kubernetes-e2e-gce-alpha-features
  • /test pull-kubernetes-e2e-gce-correctness
  • /test pull-kubernetes-e2e-gce-csi-serial
  • /test pull-kubernetes-e2e-gce-device-plugin-gpu
  • /test pull-kubernetes-e2e-gce-iscsi
  • /test pull-kubernetes-e2e-gce-iscsi-serial
  • /test pull-kubernetes-e2e-gce-kubetest2
  • /test pull-kubernetes-e2e-gce-network-proxy-grpc
  • /test pull-kubernetes-e2e-gce-storage-disruptive
  • /test pull-kubernetes-e2e-gce-storage-slow
  • /test pull-kubernetes-e2e-gce-storage-snapshot
  • /test pull-kubernetes-e2e-gci-gce-autoscaling
  • /test pull-kubernetes-e2e-gci-gce-ingress
  • /test pull-kubernetes-e2e-gci-gce-ipvs
  • /test pull-kubernetes-e2e-iptables-azure-dualstack
  • /test pull-kubernetes-e2e-ipvs-azure-dualstack
  • /test pull-kubernetes-e2e-kind-canary
  • /test pull-kubernetes-e2e-kind-dual-canary
  • /test pull-kubernetes-e2e-kind-ipv6-canary
  • /test pull-kubernetes-e2e-kind-ipvs-dual-canary
  • /test pull-kubernetes-e2e-kind-multizone
  • /test pull-kubernetes-e2e-kops-aws
  • /test pull-kubernetes-e2e-ubuntu-gce-network-policies
  • /test pull-kubernetes-e2e-windows-gce
  • /test pull-kubernetes-kubemark-e2e-gce-big
  • /test pull-kubernetes-local-e2e
  • /test pull-kubernetes-node-crio-cgrpv2-e2e
  • /test pull-kubernetes-node-crio-cgrpv2-e2e-kubetest2
  • /test pull-kubernetes-node-crio-e2e
  • /test pull-kubernetes-node-crio-e2e-kubetest2
  • /test pull-kubernetes-node-e2e-containerd-features
  • /test pull-kubernetes-node-e2e-containerd-features-kubetest2
  • /test pull-kubernetes-node-e2e-containerd-kubetest2
  • /test pull-kubernetes-node-kubelet-serial-containerd
  • /test pull-kubernetes-node-kubelet-serial-containerd-kubetest2
  • /test pull-kubernetes-node-kubelet-serial-cpu-manager
  • /test pull-kubernetes-node-kubelet-serial-cpu-manager-kubetest2
  • /test pull-kubernetes-node-kubelet-serial-crio-cgroupv1
  • /test pull-kubernetes-node-kubelet-serial-crio-cgroupv2
  • /test pull-kubernetes-node-kubelet-serial-hugepages
  • /test pull-kubernetes-node-kubelet-serial-memory-manager
  • /test pull-kubernetes-node-kubelet-serial-topology-manager
  • /test pull-kubernetes-node-kubelet-serial-topology-manager-kubetest2
  • /test pull-kubernetes-node-memoryqos-cgrpv2
  • /test pull-kubernetes-node-swap-fedora
  • /test pull-kubernetes-unit-experimental
  • /test pull-publishing-bot-validate

Use /test all to run the following jobs that were automatically triggered:

  • pull-kubernetes-conformance-kind-ga-only-parallel
  • pull-kubernetes-conformance-kind-ipv6-parallel
  • pull-kubernetes-dependencies
  • pull-kubernetes-e2e-gce-100-performance
  • pull-kubernetes-e2e-gce-ubuntu-containerd
  • pull-kubernetes-e2e-kind
  • pull-kubernetes-e2e-kind-ipv6
  • pull-kubernetes-integration
  • pull-kubernetes-node-e2e-containerd
  • pull-kubernetes-typecheck
  • pull-kubernetes-unit
  • pull-kubernetes-verify
  • pull-kubernetes-verify-govet-levee

In response to this:

/test ?

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.

@matthyx
Copy link
Contributor Author

matthyx commented Jan 27, 2022

/test pull-kubernetes-node-e2e-containerd

@matthyx
Copy link
Contributor Author

matthyx commented Jan 27, 2022

@bobbypage any idea on how to run these tests from CI?

@bobbypage
Copy link
Member

bobbypage commented Jan 27, 2022 •
edited
Loading

They should run as part of serial tests - https://testgrid.k8s.io/sig-node-release-blocking#node-kubelet-serial-containerd&include-filter-by-regex=NodeFeature:GracefulNodeShutdown

/test pull-kubernetes-node-kubelet-serial-containerd

looks promising

@matthyx
Copy link
Contributor Author

matthyx commented Jan 27, 2022

/test pull-kubernetes-node-kubelet-serial-containerd
Let's try, cheers David!

@matthyx
Copy link
Contributor Author

matthyx commented Jan 28, 2022

/test pull-kubernetes-node-kubelet-serial-containerd

@matthyx
Copy link
Contributor Author

matthyx commented Jan 28, 2022

/test pull-kubernetes-node-kubelet-serial-containerd

@matthyx
Copy link
Contributor Author

matthyx commented Jan 28, 2022

@bobbypage bummer... looks like the version of busctl in cos is too old and doesn't include emit command:

W0128 13:20:21.738] Jan 28 12:37:26.883: INFO: Matthias, here is the output of busctl: busctl [OPTIONS...] {COMMAND} ...
W0128 13:20:21.738] 
W0128 13:20:21.738] Introspect the bus.
W0128 13:20:21.739] 
W0128 13:20:21.739]   -h --help               Show this help
W0128 13:20:21.739]      --version            Show package version
W0128 13:20:21.739]      --no-pager           Do not pipe output into a pager
W0128 13:20:21.739]      --no-legend          Do not show the headers and footers
W0128 13:20:21.739]      --system             Connect to system bus
W0128 13:20:21.739]      --user               Connect to user bus
W0128 13:20:21.739]   -H --host=[USER@]HOST   Operate on remote host
W0128 13:20:21.740]   -M --machine=CONTAINER  Operate on local container
W0128 13:20:21.740]      --address=ADDRESS    Connect to bus specified by address
W0128 13:20:21.740]      --show-machine       Show machine ID column in list
W0128 13:20:21.740]      --unique             Only show unique names
W0128 13:20:21.740]      --acquired           Only show acquired names
W0128 13:20:21.740]      --activatable        Only show activatable names
W0128 13:20:21.740]      --match=MATCH        Only show matching messages
W0128 13:20:21.740]      --size=SIZE          Maximum length of captured packet
W0128 13:20:21.740]      --list               Don't show tree, but simple object path list
W0128 13:20:21.740]   -q --quiet              Don't show method call reply
W0128 13:20:21.741]      --verbose            Show result values in long format
W0128 13:20:21.741]      --expect-reply=BOOL  Expect a method call reply
W0128 13:20:21.741]      --auto-start=BOOL    Auto-start destination service
W0128 13:20:21.741]      --allow-interactive-authorization=BOOL
W0128 13:20:21.741]                           Allow interactive authorization for operation
W0128 13:20:21.741]      --timeout=SECS       Maximum time to wait for method call completion
W0128 13:20:21.741]      --augment-creds=BOOL Extend credential data with data read from /proc/$PID
W0128 13:20:21.741]      --watch-bind=BOOL    Wait for bus AF_UNIX socket to be bound in the file
W0128 13:20:21.741]                           system
W0128 13:20:21.742] 
W0128 13:20:21.742] Commands:
W0128 13:20:21.742]   list                    List bus names
W0128 13:20:21.742]   status [SERVICE]        Show bus service, process or bus owner credentials
W0128 13:20:21.742]   monitor [SERVICE...]    Show bus traffic
W0128 13:20:21.742]   capture [SERVICE...]    Capture bus traffic as pcap
W0128 13:20:21.742]   tree [SERVICE...]       Show object tree of service
W0128 13:20:21.742]   introspect SERVICE OBJECT [INTERFACE]
W0128 13:20:21.742]   call SERVICE OBJECT INTERFACE METHOD [SIGNATURE [ARGUMENT...]]
W0128 13:20:21.742]                           Call a method
W0128 13:20:21.742]   get-property SERVICE OBJECT INTERFACE PROPERTY...
W0128 13:20:21.743]                           Get property value
W0128 13:20:21.743]   set-property SERVICE OBJECT INTERFACE PROPERTY SIGNATURE ARGUMENT...
W0128 13:20:21.743]                           Set property value
W0128 13:20:21.743]   help                    Show this help

This is weird as the pr adding emit dates from 2019...

@matthyx
Copy link
Contributor Author

matthyx commented Feb 12, 2022

Since the issue is triage accepted...
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 12, 2022
@matthyx
Copy link
Contributor Author

matthyx commented Feb 13, 2022

Sending to system but rather than session might help...
/test pull-kubernetes-node-kubelet-serial-containerd

@matthyx matthyx marked this pull request as ready for review February 13, 2022 14:53
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 13, 2022
@matthyx
Copy link
Contributor Author

matthyx commented Feb 13, 2022

@bobbypage looks like I can successfully use godbus which is already used elsewhere in the kubelet to inhibit shutdown signals.
Let me know what you think?

@matthyx
Copy link
Contributor Author

matthyx commented Feb 13, 2022

/retest

@bobbypage
Copy link
Member

@bobbypage looks like I can successfully use godbus which is already used elsewhere in the kubelet to inhibit shutdown signals. Let me know what you think?

I like this idea of using godbus directly, it will ensure there is no dependency on any cli tools in the OS image.

@bobbypage
Copy link
Member

/lgtm

@bobbypage
Copy link
Member

/assign @wzshiming

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2022
@wzshiming
Copy link
Member

I like this idea of using godbus directly, it will ensure there is no dependency on any cli tools in the OS image.

Good idea, I never noticed that godbus can emit signals.

/lgtm

@wzshiming
Copy link
Member

/assign @mrunalp

SergeyKanzhelev
SergeyKanzhelev approved these changes Mar 30, 2022
View reviewed changes
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matthyx, SergeyKanzhelev

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 30, 2022
@matthyx
Copy link
Contributor Author

matthyx commented Mar 30, 2022

@SergeyKanzhelev I guess we won't put the milestone here?

@k8s-ci-robot k8s-ci-robot merged commit 6ffd13f into kubernetes:master May 4, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev approved these changes

@bobbypage bobbypage Awaiting requested review from bobbypage

@Random-Liu Random-Liu Awaiting requested review from Random-Liu

@mrunalp mrunalp Awaiting requested review from mrunalp

@odinuge odinuge Awaiting requested review from odinuge

Assignees

@mrunalp mrunalp

@bobbypage bobbypage

@wzshiming wzshiming

@SergeyKanzhelev SergeyKanzhelev

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node CI/Test Board
Archived in project
SIG Node PR Triage
Archived in project
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.

graceful node shutdown test - improve dbus command to support other distros
7 participants
@matthyx @k8s-ci-robot @bobbypage @dims @wzshiming @SergeyKanzhelev @mrunalp