-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add release spec format to selectively update containers in resources #1218
Conversation
69a44b8
to
e160b8a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The request will fail if any of the Current container image requirements
are not met. To have partial updates go through and ignore the failed
requirements, one can pass true for IgnoreFailedControllers.
This suggests that the it's only failing to meet the precondition (Current field) that can be excused by IgnoreFailedControllers
; but it looks like it's also other failures, e.g., a container being missing entirely.
It seems to me that these are qualitatively different kinds of failure: failing to meet the precondition means you have an out-of-date spec, but failing to name the right containers means you have an incorrect spec.
I'm not totally sure how this will be used, so I'd appreciate a rationale for how it works, in particular,
- why is it useful to be able to ignore a part of the update that names a missing container?
- should it fail overall if you are ignoring failures, but nothing is successful?
An alternative would be to not supply a precondition, if you don't have one -- and all preconditions that have been supplied must be met, to succeed overall. But again, I'm not sure of the intended use, so this may be less suitable.
release/releaser_test.go
Outdated
Status: update.ReleaseStatusFailed, | ||
Error: fmt.Sprintf(update.ContainerTagMismatch, helloContainer), | ||
}, | ||
Commit: "Release containers\n\ndefault:deployment/helloworld failed: container tag mismatch: greeter", |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
I've reread #1140 and I understand a bit better what the purpose of
In that case, I reckon it should switch between the mode of "succeed only if all preconditions are met" and "succeed for all containers where the preconditions are met, leave others as they are". Unconditionally updating containers (being able to not supply a precondition) is a different matter -- sorry for the distraction -- though may in itself be useful, I don't know. |
37268d2
to
9d334ae
Compare
@squaremo yes, I adjusted the logic a bit to make things clearer with regard to failed/skipped states. It's either
|
9d334ae
to
97e823e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Misnamed containers should be outright failures.
Other stuff is just advisory :-)
update/containers.go
Outdated
} | ||
|
||
// An empty spec for the current image skips the precondition | ||
empty := image.Ref{} |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
update/containers.go
Outdated
} | ||
} | ||
|
||
if !s.IgnoreMismatches && len(notfound) > 0 { |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
update/containers.go
Outdated
notFoundError := fmt.Sprintf(ContainerNotFound, strings.Join(notfound, ", ")) | ||
mismatchError := fmt.Sprintf(ContainerTagMismatch, strings.Join(mismatch, ", ")) | ||
|
||
if s.IgnoreMismatches { |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
return s.Kind | ||
} | ||
|
||
func (s ContainerSpecs) ReleaseType() ReleaseType { |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
for _, spec := range s.ContainerSpecs[u.ResourceID] { | ||
container, ok := containers[spec.Container] | ||
if !ok { | ||
notfound = append(notfound, spec.Container) |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
update/containers.go
Outdated
type ContainerSpecs struct { | ||
Kind ReleaseKind | ||
ContainerSpecs map[flux.ResourceID][]ContainerUpdate | ||
IgnoreMismatches bool |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
Looks good, let's give it a go 😄
|
This adds a new release spec format providing update requests on container level. Sample POST request body: ``` { "type": "containers", "spec": { "ContainerSpecs": { "default:deployment/nginx": [ { "Container": "nginx", "Current": "nginx:1.14.0", "Target": "nginx:1.15.0" } ], "default:deployment/helloworld": [ { "Container": "helloworld", "Current": "quay.io/weaveworks/helloworld:master-07a1b6b", "Target": "quay.io/weaveworks/helloworld:master-a000004" }, { "Container": "sidecar", "Current": "quay.io/weaveworks/sidecar:master-a000001", "Target": "quay.io/weaveworks/sidecar:master-a000002" } ] }, "Kind": "plan", "SkipMismatches": false } } ``` The request will fail if any of the Current container image requirements are not met. To have partial updates go through failures, one can pass true for IgnoreMismatches. Note that this still leads to an error if containers cannot be found. Also, `Current` can be left empty to disable any precondition check.
38071a9
to
8c6b459
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra approval for extra commit
@rndstr To which endpoint does one do the POST and with what URL params, if any? |
@jpellizzari |
This adds a new release spec format providing update requests on container
level.
Sample POST request body to
/v9/update-manifests
:The request will fail if any of the
Current
container image requirementsare not met or containers cannot be found. To have partial updates go
through and ignore any failures, one can pass
true
forIgnoreMismatches
.Also,
Current
can be left empty to disable any precondition check.Closes #1140