[merged] Add cluster subcommand#354
[merged] Add cluster subcommand#354mbarnes wants to merge 1 commit intoprojectatomic:masterfrom mbarnes:cluster-command
Conversation
|
CC @ashcrow: Extra pair of eyeballs wouldn't hurt, if you're interested. |
|
@mbarnes looks good to me, though the last two commits might be squashable as they are editing the same lines. |
bash/atomic
Outdated
|
|
||
| } | ||
|
|
||
| _atomic_cluster_no_opts() { |
There was a problem hiding this comment.
Can you create this in Atomic/cluster.py?
There was a problem hiding this comment.
@rhatdan I can, but are you sure? Nothing else under Atomic/ appears to be serving bash completion. Just wanted to double check first.
There was a problem hiding this comment.
Never mind, I though this was part of the atomic command not the bash completions.
|
How about this patch for running atomic as non root. |
Looks like it already got merged, but I'm happy with it too. I'll rebase this branch. |
docs/atomic-cluster.1.md
Outdated
|
|
||
| # SUBCOMMANDS | ||
| **get cluster** CLUSTER_NAME | ||
| Report status of the specified cluster. |
There was a problem hiding this comment.
Shouldn't this be just get not get cluster
atomic cluster get CLUSTER_NAME
There was a problem hiding this comment.
Hmm... I agree that particular case reads better. But there's some other types of things you can "get" (or "create" or "delete") besides cluster objects and I'm worried it would make parsing tricky -- or even ambiguous if we add more types of things to "get" in the future.
An easier solution to the redundancy is to rename the atomic cluster part, though the best I can think of is atomic multihost.
@ashcrow: Any thoughts here?
There was a problem hiding this comment.
There are other options to commctl'scluster command (get, create, delete). However, seeing cluster get cluster $NAME does seem odd.
A few ideas:
- Maybe it would make better sense to use a different direct command than
cluster. Maybemanage? - Maybe we can flatten out the commands we add so that they are at a higher level like:
atomic cluster get $NAME
atomic host delete $NAME
atomic status # maybe this one is a special case where we say atomic manager status
...There was a problem hiding this comment.
Perhaps we should setup a bluejeans to discuss this. I really do not understand what Commissaire does, so tough for me to give opinion.
There was a problem hiding this comment.
Maybe we can flatten out the commands we add so that they are at a higher level like:
atomic cluster get $NAME
atomic host delete $NAME
atomic status # maybe this one is a special case where we say atomic manager status
That's going to trample on the atomic namespace too much I think. atomic host, for example, is already a thing (wraps rpm-ostree). I'm on board with your first suggestion though, but picking a good name is hard.
One thing that maybe wasn't stated explicitly enough in #329 was that we already have a standalone command-line tool for this named commctl and it shares the same command syntax (and code) with what we're proposing for atomic cluster.
IOW, atomic cluster get cluster == commctl get cluster.
But it's not clear to me (or even @ashcrow I think) whether commctl is meant to be kept if/when this stuff lands in atomic.
|
☔ The latest upstream changes (presumably #359) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@rhatdan, @ashcrow: I know we agreed on a syntax when we chatted, but the Currently I think conflating local vs remote actions into the same @ashcrow and I were tossing around more ideas and we hit upon making it more explicit that a subset of What do you think about these? (@ashcrow, I took some liberties here vs what's in |
|
Seems sane to me. On a semi-related topic, Cockpit ships |
|
Is the cluster remote Or just the host? Not crazy about 4 commands. |
You can add hosts to a cluster manually, but I believe the intent is for a cluster node to add itself on boot via cloud-init (which just uses Python libraries) and reserve the
Yeah, it's a little wordy. Would hyphenating help? |
|
How about |
|
rhost sounds good to me. @mbarnes correct, the commands |
|
I'm good with that too. It will require a bit of rework on the |
|
Any update? |
|
Not quite yet. Got side tracked, but I'm back on this now. |
|
@rhatdan Before I start on docs / bash completion updates here, can you take a look at projectatomic/commctl#12? That shows the new syntax for
|
|
That looks good to me, except for the |
That one seems like a poor fit for Sound alright? |
|
Fine with me. Your the boss. |
|
I'm fine with keeping |
|
⬆️ Complete rewrite works with latest (which the Jenkins environment appears to be missing) |
|
Rebased and updated bash completion + manpages for a new Commissaire operation coming soon. (projectatomic/commissaire#135) I don't know how to correct for the Jenkins failure. The |
|
@jlebon How do we get the latest version of commictl into the test environment? |
|
Shouldn't the import and cli parsers be conditional on commissaire being present? E.g. like we did for the recent system containers feature and ostree. |
I had it conditional at first and some discussion ensued. See outdated patch review comments above from Apr 23 (can't seem to link to them directly...) |
|
Yup, commented there as well, but you're right, the link to it doesn't actually bring you to the discussion. Reposting here to make it easier:
|
|
I am fine with going back to making it optional, just not crazy about man pages being available but functionality not. But we set the president of ostree, so I guess we can follow it here. |
|
Talking about this in terms of Jenkins is mostly a red herring - there's two types of dependencies:
(And each type can be either hard or soft/optional) The current internal Jenkins PR tester conflates these, but the rpmdistro-gitoverlay builder does not, it uses RPM semantics. (On the other hand it doesn't yet run per-project One major pain point specific to this project is the way we extract the version number from the python code is that it turns all hard runtime dependencies into build-time too. Philosophy aside it does seem like having the dep be soft/optional at runtime makes sense. |
|
⬆️ Jenkins is happy again. We could revisit the optional aspect once |
| # SYNOPSIS | ||
| **atomic cluster [OPTIONS] SUBCOMMAND** | ||
|
|
||
| **atomic cluster** will issue commands to a Commissaire server over HTTP. |
There was a problem hiding this comment.
Maybe a small line here to say that it will only be available if Commissaire is installed?
There was a problem hiding this comment.
I'm not sure where manpages get packaged for AH these days but my thought was if we could temporarily delete these two pages in a spec file or whatever, that seems cleaner to me than a disclaimer. Failing that, yeah I can add a line.
There was a problem hiding this comment.
They are packaged up in the RHELtools container
|
What is the latest on this pull request? |
Interfaces with a Commissaire server. https://github.com/projectatomic/commissaire
|
⬆️ I added the man page disclaimers @jlebon suggested, rebased on current |
|
👍 |
|
☀️ Test successful - status-atomicjenkins |
7 participants
This adds a
clustersubcommand to integrate with Commissaire, as discussed in #329.One thing to note about
atomic clusteris it does not require root privilege; it's just talking over HTTP. I tried to solve that cleanly in followup commits by delaying the root user check until after the command line is parsed and then adding a way for subcommands to opt out of requiring the root user. This has the added benefit of letting non-root users at least get--helpoutput for anyatomiccommand.