Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Add release spec format to selectively update containers in resources #1218

Merged
merged 2 commits into from
Jul 12, 2018

Conversation

rndstr
Copy link
Contributor

@rndstr rndstr commented Jul 9, 2018

This adds a new release spec format providing update requests on container
level.

Sample POST request body to /v9/update-manifests:

{
  "type": "containers",
  "cause": {
    "Message": "sample request body",
    "User": "alice"
  },
  "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",
    "IgnoreMismatches": false
  }
}

The request will fail if any of the Current container image requirements
are not met or containers cannot be found. To have partial updates go
through and ignore any failures, one can pass true for IgnoreMismatches.

Also, Current can be left empty to disable any precondition check.

Closes #1140

Copy link
Member

@squaremo squaremo left a 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.

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.

This comment was marked as abuse.

@squaremo
Copy link
Member

I've reread #1140 and I understand a bit better what the purpose of IgnoreFailedControllers is -- it's to address this bit, I think:

You might find that, for example, releases can hardly ever succeed, because something always changes before you hit the button. In that case it would be useful to be able to relax the all-or-nothingness

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.

@rndstr rndstr force-pushed the issue/1140-container-release branch from 37268d2 to 9d334ae Compare July 11, 2018 02:38
@rndstr
Copy link
Contributor Author

rndstr commented Jul 11, 2018

@squaremo yes, I adjusted the logic a bit to make things clearer with regard to failed/skipped states.

It's either

  • only update if all preconditions match (default)
  • update all containers whose preconditions match (IgnoreMismatches–open to renaming)
  • skip preconditions (don't provide them)

@rndstr rndstr force-pushed the issue/1140-container-release branch from 9d334ae to 97e823e Compare July 12, 2018 15:16
Copy link
Member

@squaremo squaremo left a 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 :-)

}

// An empty spec for the current image skips the precondition
empty := image.Ref{}

This comment was marked as abuse.

}
}

if !s.IgnoreMismatches && len(notfound) > 0 {

This comment was marked as abuse.

notFoundError := fmt.Sprintf(ContainerNotFound, strings.Join(notfound, ", "))
mismatchError := fmt.Sprintf(ContainerTagMismatch, strings.Join(mismatch, ", "))

if s.IgnoreMismatches {

This comment was marked as abuse.

return s.Kind
}

func (s ContainerSpecs) ReleaseType() ReleaseType {

This comment was marked as abuse.

This comment was marked as abuse.

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.

type ContainerSpecs struct {
Kind ReleaseKind
ContainerSpecs map[flux.ResourceID][]ContainerUpdate
IgnoreMismatches bool

This comment was marked as abuse.

@squaremo
Copy link
Member

Looks good, let's give it a go 😄

git rebase -i master to get rid of the feedback commits, depending on your personal attitude towards git neat-and-tidiness (and present level of busyness).

rndstr added 2 commits July 12, 2018 09:39
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.
@rndstr rndstr force-pushed the issue/1140-container-release branch from 38071a9 to 8c6b459 Compare July 12, 2018 16:44
Copy link
Member

@squaremo squaremo left a 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 rndstr merged commit 1b6442d into master Jul 12, 2018
@rndstr rndstr deleted the issue/1140-container-release branch July 12, 2018 16:49
@jpellizzari
Copy link
Contributor

@rndstr To which endpoint does one do the POST and with what URL params, if any?

@rndstr
Copy link
Contributor Author

rndstr commented Jul 23, 2018

@jpellizzari /v9/update-manifests; no URL params, all the options are in the JSON body

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants