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

Provide error handling for Reboot() call #387

Open
invidian opened this issue Jan 6, 2022 · 4 comments · May be fixed by #390
Open

Provide error handling for Reboot() call #387

invidian opened this issue Jan 6, 2022 · 4 comments · May be fixed by #390

Comments

@invidian
Copy link

invidian commented Jan 6, 2022

Right now user has no way of knowing if calling Reboot() succeeds or not, as no error is returned from a call. E.g. if user calling it has no permissions to do so, the call will silently fail.

This could be improved, e.g. together with accepting the context for operation in new function.
invidian added a commit to flatcar/flatcar-linux-update-operator that referenced this issue Jan 6, 2022
@invidian
pkg/login1: initial commit
f376d66 
This commit adds login1 package, which is a small subset of
github.com/coreos/go-systemd/v22/login1 package with ability to use
shared D-Bus connection and with proper error handling for Reboot method
call, which is not yet provided by the upstream.

The idea is to use this package in favor of github.com/coreos/go-systemd
in agent code responsible for rebooting the node. However, this requires
tests in agent code, so it will be done in the next step.

See coreos/go-systemd#387 for more details.

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
@invidian invidian mentioned this issue Jan 6, 2022
Open
@lucab
Copy link
Contributor

lucab commented Jan 6, 2022

Good idea, a RebootWithContext would fit nicely. Would you like to submit a PR with that?

@invidian
Copy link
Author

invidian commented Jan 6, 2022

Can't promise I find time to do so, but will try :)

@invidian
Copy link
Author

invidian commented Jan 6, 2022

Ah, just found out there is more general issue for this: #326, not sure if it make sense to address each method individually or should we handle all of them at once.

invidian added a commit to flatcar/flatcar-linux-update-operator that referenced this issue Jan 6, 2022
@invidian
pkg/login1: initial commit
12a4b2a 
This commit adds login1 package, which is a small subset of
github.com/coreos/go-systemd/v22/login1 package with ability to use
shared D-Bus connection and with proper error handling for Reboot method
call, which is not yet provided by the upstream.

The idea is to use this package in favor of github.com/coreos/go-systemd
in agent code responsible for rebooting the node. However, this requires
tests in agent code, so it will be done in the next step.

See coreos/go-systemd#387 for more details.

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
invidian added a commit to flatcar/flatcar-linux-update-operator that referenced this issue Jan 6, 2022
@invidian
pkg/login1: initial commit
0d29d06 
This commit adds login1 package, which is a small subset of
github.com/coreos/go-systemd/v22/login1 package with ability to use
shared D-Bus connection and with proper error handling for Reboot method
call, which is not yet provided by the upstream.

The idea is to use this package in favor of github.com/coreos/go-systemd
in agent code responsible for rebooting the node. However, this requires
tests in agent code, so it will be done in the next step.

See coreos/go-systemd#387 for more details.

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
invidian added a commit to flatcar/flatcar-linux-update-operator that referenced this issue Jan 6, 2022
@invidian
pkg/login1: initial commit
d67e745 
This commit adds login1 package, which is a small subset of
github.com/coreos/go-systemd/v22/login1 package with ability to use
shared D-Bus connection and with proper error handling for Reboot method
call, which is not yet provided by the upstream.

The idea is to use this package in favor of github.com/coreos/go-systemd
in agent code responsible for rebooting the node. However, this requires
tests in agent code, so it will be done in the next step.

See coreos/go-systemd#387 for more details.

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
invidian added a commit to flatcar/flatcar-linux-update-operator that referenced this issue Jan 6, 2022
@invidian
pkg/login1: initial commit
3d80210 
This commit adds login1 package, which is a small subset of
github.com/coreos/go-systemd/v22/login1 package with ability to use
shared D-Bus connection and with proper error handling for Reboot method
call, which is not yet provided by the upstream.

The idea is to use this package in favor of github.com/coreos/go-systemd
in agent code responsible for rebooting the node. However, this requires
tests in agent code, so it will be done in the next step.

See coreos/go-systemd#387 for more details.

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
invidian added a commit to kinvolk/go-systemd that referenced this issue Jan 10, 2022
@invidian
login1: add RebootWithContext method
95c855f 
Existing Reboot() method does not allow using context not inspecting
D-Bus call errors, which makes it difficult to debug and use.

This commit adds new RebootWithContext() method which addresses those
shortcomings.

Closes coreos#387

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
invidian added a commit to kinvolk/go-systemd that referenced this issue Jan 10, 2022
@invidian
login1: add RebootWithContext method
0722452 
Existing Reboot() method does not allow using context not inspecting
D-Bus call errors, which makes it difficult to debug and use.

This commit adds new RebootWithContext() method which addresses those
shortcomings.

Closes coreos#387

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
@invidian invidian linked a pull request Jan 10, 2022 that will close this issue
Open
invidian added a commit to kinvolk/go-systemd that referenced this issue Jan 10, 2022
@invidian
login1: Add RebootWithContext method
b3aa1e3 
Existing Reboot() method does not allow using context not inspecting
D-Bus call errors, which makes it difficult to debug and use.

This commit adds new RebootWithContext() method which addresses those
shortcomings.

Closes coreos#387

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
@invidian
Copy link
Author

Created #390 to address this.

invidian added a commit to kinvolk/go-systemd that referenced this issue Jan 10, 2022
@invidian
login1: Add RebootWithContext method
484c87e 
Existing Reboot() method does not allow using context not inspecting
D-Bus call errors, which makes it difficult to debug and use.

This commit adds new RebootWithContext() method which addresses those
shortcomings.

Closes coreos#387

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
invidian added a commit to kinvolk/go-systemd that referenced this issue Jan 10, 2022
@invidian
login1: Add RebootWithContext method
1a186bc 
Existing Reboot() method does not allow using context not inspecting
D-Bus call errors, which makes it difficult to debug and use.

This commit adds new RebootWithContext() method which addresses those
shortcomings.

Closes coreos#387

Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

login1: add NewWithConnection and RebootWithContext methods kinvolk/go-systemd
2 participants
@lucab @invidian