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

Convert SpecGen values to be nullable where possible #21339

Merged
merged 2 commits into from
Jan 30, 2024

Conversation

mheon
Copy link
Member

@mheon mheon commented Jan 23, 2024

SpecGen is our primary container creation abstraction, and is used to connect our CLI to the Libpod container creation backend. Because container creation has a million options (I exaggerate only slightly), the struct is composed of several other structs, many of which are quite large.

The core problem is that SpecGen is also an API type - it's used in remote Podman. There, we have a client and a server, and we want to respect the server's containers.conf. But how do we tell what parts of SpecGen were set by the client explicitly, and what parts were not? If we're not using nullable values, an explicit empty string and a value never being set are identical - and we can't tell if it's safe to grab a default from the server's containers.conf.

Fortunately, we only really need to do this for booleans. An empty string is sufficient to tell us that a string was unset (even if the user explicitly gave us an empty string for an option, filling in a default from the config file is acceptable). This makes things a lot simpler. My initial attempt at this changed everything, including strings, and it was far larger and more painful.

Also, begin the first steps of removing all uses of containers.conf defaults from client-side. Two are gone entirely, the rest are marked as remove-when-possible.

Marking as release note none for now, because this may not actually be a breaking change to our API, given how lenient Go's JSON parsing has always been.

[NO NEW TESTS NEEDED] This is just a refactor.

Does this PR introduce a user-facing change?

NONE

Copy link
Contributor

openshift-ci bot commented Jan 23, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2024
@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Jan 23, 2024
Copy link

Cockpit tests failed for commit 5e2b2407743adf0aecba4248e6ba2e8586acc65e. @martinpitt, @jelly, @mvollmer please check.

Copy link

Cockpit tests failed for commit f1ab0e70525b8fe7717e18c989bdfc168233d9cf. @martinpitt, @jelly, @mvollmer please check.

Copy link

Cockpit tests failed for commit a9e924e56854a1730acc6eb6b4b3186cce524458. @martinpitt, @jelly, @mvollmer please check.

@martinpitt
Copy link
Contributor

The cockpit failure here is much less severe than in #21308.

wait_js_cond(ph_in_text(".pf-v5-c-alert","address already in use")): actual text: Danger alert:Container failed to be created Error message: Internal Server Error: invalid config provided: invalid ImageVolumeMode "bind", value must be one of ignore,tmpfs,anonymous

This test creates a first container with exposing port 5000 on the CLI ( -p 5000:5000) and then tries to create a container in the UI with also exposing port 5000, and expects that "address already in use" error message. Instead it fails with that curious one -- bind should certainly be a valid volume mode, and it does work in other cases as well?

Is that enough for you to spot the error, or do you need a curl reproducer?

@mheon
Copy link
Member Author

mheon commented Jan 24, 2024

containers/common#1820 (once merged and vendored) will fix the image-volume issue. The containers.conf default is not sane and needs to be changed.

@mheon
Copy link
Member Author

mheon commented Jan 29, 2024

Vendored our updated c/common

Copy link

Cockpit tests failed for commit d7378f97e0c8ec93a554c020f11c18d6c1f4d684. @martinpitt, @jelly, @mvollmer please check.

@ashley-cui
Copy link
Member

/hold
/lgtm

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2024
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2024
Copy link

Cockpit tests failed for commit 885c65319254a9f97af8158a2739ebdcfc5734c8. @martinpitt, @jelly, @mvollmer please check.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2024
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@mheon mheon force-pushed the specgen_to_pointer_v2 branch 3 times, most recently from a7bebd0 to 7526363 Compare January 30, 2024 03:56
mheon and others added 2 commits January 30, 2024 10:42
SpecGen is our primary container creation abstraction, and is
used to connect our CLI to the Libpod container creation backend.
Because container creation has a million options (I exaggerate
only slightly), the struct is composed of several other structs,
many of which are quite large.

The core problem is that SpecGen is also an API type - it's used
in remote Podman. There, we have a client and a server, and we
want to respect the server's containers.conf. But how do we tell
what parts of SpecGen were set by the client explicitly, and what
parts were not? If we're not using nullable values, an explicit
empty string and a value never being set are identical - and we
can't tell if it's safe to grab a default from the server's
containers.conf.

Fortunately, we only really need to do this for booleans. An
empty string is sufficient to tell us that a string was unset
(even if the user explicitly gave us an empty string for an
option, filling in a default from the config file is acceptable).
This makes things a lot simpler. My initial attempt at this
changed everything, including strings, and it was far larger and
more painful.

Also, begin the first steps of removing all uses of
containers.conf defaults from client-side. Two are gone entirely,
the rest are marked as remove-when-possible.

[NO NEW TESTS NEEDED] This is just a refactor.

Signed-off-by: Matt Heon <mheon@redhat.com>
Signed-off-by: Matthew Heon <matthew.heon@pm.me>
@baude
Copy link
Member

baude commented Jan 30, 2024

/lgtm

hold is already present

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2024
@baude
Copy link
Member

baude commented Jan 30, 2024

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 30, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit dabbcb8 into containers:main Jan 30, 2024
91 of 92 checks passed
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Apr 30, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Change to remote API; merits scrutiny lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants