-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
cgroups/systemd: replace deprecated dbus functions #2926
cgroups/systemd: replace deprecated dbus functions #2926
Conversation
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@@ -321,7 +322,7 @@ func isUnitExists(err error) bool { | |||
func startUnit(cm *dbusConnManager, unitName string, properties []systemdDbus.Property) error { | |||
statusChan := make(chan string, 1) | |||
err := cm.retryOnDisconnect(func(c *systemdDbus.Conn) error { | |||
_, err := c.StartTransientUnit(unitName, "replace", properties, statusChan) | |||
_, err := c.StartTransientUnitContext(context.TODO(), unitName, "replace", properties, statusChan) |
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.
Possibly we can use a context.WithTimeout()
to replace the handling below, but I didn't want to make larger changes, because the last changes were fixing a bug, so didn't want to jinx it.
/cc @kolyshkin
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.
I was taking a look at it earlier this week, and I am not sure if the timeout can be replaced with a context.WithTimeout()
. I read the code and don't quite understand it.
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.
context.TODO()
is a little bit 😬 but we'd need to change all of the callers to pass context
if we wanted to change it, so we can do that some other time.
Agreed; I picked Do you want me to open a tracking issue for that effort? |
I was preparing a similar PR but dropped it, as the changes introduced basically do two things
Yes please (I guess it's inevitable we add context to libcontainer's public API). |
Actually this is not true because coreos/go-systemd#340 and coreos/go-systemd#341 did not put "Deprecated:" as a separate paragraph (as required by and described in https://github.com/golang/go/wiki/Deprecated). This is why there are no linter warnings (or pkg.go.dev warnings, for that matter). |
These are deprecated, but pkg.go.dev does not make that very clear; https://pkg.go.dev/github.com/coreos/go-systemd/v22/dbus#Conn.StopUnitContext
It also does a really lame job at pointing out that you're reading the "latest version" of an "ancient" release
relates to #2923