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

cgroups/systemd: replace deprecated dbus functions #2926

Merged

Conversation

thaJeztah
Copy link
Member

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

Screenshot 2021-04-29 at 22 05 26

relates to #2923

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@@ -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)
Copy link
Member Author

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

Copy link
Contributor

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.

Copy link
Member

@cyphar cyphar left a 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.

@cyphar cyphar merged commit 016717a into opencontainers:master Apr 30, 2021
@thaJeztah thaJeztah deleted the replace_deprecated_dbus_functions branch April 30, 2021 08:39
@thaJeztah
Copy link
Member Author

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

Agreed; I picked context.TODO() for that reason; it should be an indicator that something needs to be done 😉

Do you want me to open a tracking issue for that effort?

@kolyshkin
Copy link
Contributor

I was preparing a similar PR but dropped it, as the changes introduced basically do two things

  • fix linter warnings (replacing them with TODO);
  • make backports harder.

Do you want me to open a tracking issue for that effort?

Yes please (I guess it's inevitable we add context to libcontainer's public API).

@kolyshkin
Copy link
Contributor

kolyshkin commented Apr 30, 2021

fix linter warnings (replacing them with TODO)

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).

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.

4 participants