Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

fleetctl: status and restart commands with global units #1624

Merged

Conversation

dongsupark
Copy link
Contributor

@dongsupark dongsupark commented Jul 6, 2016

This PR is yet another revision of a long-running issue, fleetctl restart and status with global units.

  • First allow "fleetctl status" to run with global units. For the purpose, implement a helper cmdGlobalMachineState() that filters a list of machines only with unique entries.
  • Introduce a new command "fleetctl restart". This part is originally taken from fleetctl: Adding restart command #1238 written by "Kyle Boorky kboorky@turbine.com", committed by @hhoover .
  • Introduce unit test and functional test for fleetctl restart.
  • Add new tests for global tests in TestScheduleGlobalUnits().

This PR is not about changing API or metadata format in etcd registry. It's not introducing any new mechanism, because I don't think I can afford changing such design at the moment. It's rather a compromise between practical requirements and the current design of fleet. Though we still need to think about how to decrease number of ssh connections when restarting global units, as suggested by @kayrus. See also #1559.

Fixes #975

@simonvanderveldt
Copy link

@dongsupark have you been using this since you created this PR? How's it holding up?
This will only allow for restarting existing units and doesn't enable updating an existing global unit one by one, right?

We've been doing something similar outside of fleet using SSH connections and calling systemctl, but had some issues with fleet getting in a broken state when updating global units, which seemed to be caused by the fact that we weren't updating the desired state of the units in fleet (ie we submitted a new one using fleet and then started it using systemctl without doing a fleetctl start).

Is anyone planning to review and merge this in the near future?

@dongsupark
Copy link
Contributor Author

@simonvanderveldt
I'm just waiting for anyone to review this PR. And I thought it wouldn't be that urgent issue.
I'm also aware that in the past not everyone was happy with supporting the restart command. So I wouldn't just merge this PR, unless anyone could review it.

@dongsupark dongsupark force-pushed the dongsu/fleetctl-restart-global-units branch from 64f8b0e to fe560b0 Compare November 10, 2016 10:31
@dongsupark dongsupark force-pushed the dongsu/fleetctl-restart-global-units branch 4 times, most recently from d591ce2 to de2fcc7 Compare November 15, 2016 17:03
@dongsupark dongsupark force-pushed the dongsu/fleetctl-restart-global-units branch 2 times, most recently from 0e864e0 to f77faa3 Compare November 16, 2016 10:11
Dongsu Park added 6 commits December 14, 2016 11:49
fleetctl status should handle machine states of global units, instead of
failing with errors. To avoid creating individual ssh connection for
each machine for global unit, global units need to be first inserted into
a list of global units. They are filtered again to contain only unique
pairs of (unit name, machine ID), before fleetctl finally initiates
connection to each of the machines.
Fleetctl restart allows users to easily restart units on every machine,
without having to run manually like "fleetctl ssh systemctl restart".
Both global units and non-global units are supported.

Originally written by Kyle Boorky <kboorky@turbine.com>
See also coreos#1238
Now that the "fleetctl restart" command is available, a new unit test
is also necessary correspondently.
For start command, we don't need to explicitly check for systemd
active states, as fleetctl start is already doing that.
A new functional test TestUnitRestart is also needed to verify that
the new command "fleetctl restart" works well.
Now that fleetctl status & restart is able to run with global units,
TestScheduleGlobalUnits() needs to run "fleetctl status" as well as
"fleetctl restart" also with a global unit, to verify that the new
functionality is working too.
@dongsupark dongsupark force-pushed the dongsu/fleetctl-restart-global-units branch from f77faa3 to 060abbf Compare December 14, 2016 11:13
@dongsupark
Copy link
Contributor Author

Fixed minor things, and force-pushed.
If no one objects, I'd like to merge it before the release v1.0, to close #1559.

@dongsupark dongsupark added this to the v1.0.0 milestone Dec 14, 2016
@dongsupark dongsupark merged commit bc4201c into coreos:master Dec 16, 2016
@dongsupark dongsupark deleted the dongsu/fleetctl-restart-global-units branch December 16, 2016 06:58
dongsupark pushed a commit that referenced this pull request Dec 16, 2016
…-units

fleetctl: status and restart commands with global units
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants