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

Add HostType, change status to always succeed #244

Merged
merged 1 commit into from
Jan 2, 2024

Conversation

cgwalters
Copy link
Collaborator

@cgwalters cgwalters commented Dec 20, 2023

This depends on #241


Add HostType, change status to always succeed

Closes: #242

Basically it'd be useful for folks to be able to run bootc status --json
and have that always work - if we're not on a bootc-booted system
then we just output null data.

Specifically we drop the isContainer field and replace it
with an optional non-exhaustive enumeration. For anything we don't
know about we just output None i.e. null in JSON.


Copy link

openshift-ci bot commented Dec 20, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@cgwalters
Copy link
Collaborator Author

For reference on a non-bootc enabled host the output looks like

note: The format of this API is not yet stable
apiVersion: org.containers.bootc/v1alpha1
kind: BootcHost
metadata:
  name: host
spec:
  image: null
status:
  staged: null
  booted: null
  rollback: null
  type: null

With the important bit here being type: null.

Whereas on a bootc-enabled system it will look like e.g.:

apiVersion: org.containers.bootc/v1alpha1
kind: BootcHost
metadata:
  name: host
spec:
  image:
    image: quay.io/example/someos:latest
    transport: registry
status:
  # (omitted stuff)
  type: bootcHost

Though one corner case right now is on an ostree-but-not-bootc system (e.g. current FCOS,Fedora IoT) we show e.g.

apiVersion: org.containers.bootc/v1alpha1
kind: BootcHost
metadata:
  name: host
spec:
  image: null
status:
  staged: null
  booted:
    image: null
    incompatible: false
    pinned: false
    ostree:
      checksum: 380d2669f1726477cc6f28e07e0a7200855705d00f65a92f888a480ddd133d73
      deploySerial: 0
  rollback: null
  type: bootcHost

Which is probably wrong. Will fix that.

@beav
Copy link

beav commented Dec 20, 2023

This looks good to me. The logic would be something like:

  • if bootc executable does not exist, its not bootc managed
  • if bootc executable exists, execute and read ["status"]["type"]. If it's null, its not bootc managed
  • If you got here, its bootc managed. Read the rest of the status output for useful information

Does that sound right?

@jeckersb
Copy link
Contributor

* if bootc executable exists, execute and read `["status"]["type"]`. If it's null, its not bootc managed

Probably better to match on this being "bootcHost" instead of non-null, since under the current state this could also return "bootcContainer" (though you shouldn't see that from a "general observability" perspective). But the more general concern is that the enumeration is non-exhaustive, so in the future could contain new states like (totally contrived example) bootcTransitioning where the system is in the process of becoming managed by bootc but not quite completed. You probably don't want to consider that to be "bootc managed" yet.

@beav
Copy link

beav commented Dec 20, 2023

* if bootc executable exists, execute and read `["status"]["type"]`. If it's null, its not bootc managed

Probably better to match on this being "bootcHost" instead of non-null

works for me

lib/src/spec.rs Outdated Show resolved Hide resolved
Closes: containers#242

Basically it'd be useful for folks to be able to run `bootc status --json`
and have that always work - if we're not on a bootc-booted system
then we just output null data.

Specifically we drop the `isContainer` field and replace it
with an optional non-exhaustive enumeration.  For anything we don't
know about we just output `None` i.e. `null` in JSON.

Signed-off-by: Colin Walters <walters@verbum.org>
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vrothberg vrothberg merged commit d2a2326 into containers:main Jan 2, 2024
10 checks passed
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.

None yet

4 participants