Skip to content

Commit 58356de

Browse files
committed
*: Port from Update to Release for ClusterVersion status
The next/last Actual comparison avoids: invalid operation: next.Actual == last.Actual (struct containing []string cannot be compared) The image pullspec is sufficient for that matching. The test suite's expected LastProgress changes are because the shift from: {Version: "4.0.1", Image: "image/image:1"} to: {Version: "1.0.0-abc", Image: "image/image:1"} no longer counts as a LastProgress-bumping change (because we're now only comparing the Image pullspecs). Also adjusts the upstream/Cincinnati retrieval and payload loading logic to preserve the additional metadata that we're now exposing. Storing a Release on the Operator, instead of extending the previous releaseVersion/releaseImage pattern, makes it easy to keep track of payload-loaded metadata, so we can fall-back to upstream metadata for properties where the payload has no opinion, as described in the enhancement. In findUpdateFromConfigVersion, if some crazy upstream service declared a node with a matching version but no image, we used to say "sorry, no match" without looking through history. Now we skip the useless entry and move on to consider later entries and history. We only say "no match" if we have no image pullspec associated with the requested version available.
1 parent 7bae18c commit 58356de

File tree

22 files changed

+1664
-795
lines changed

22 files changed

+1664
-795
lines changed

docs/user/status.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ It could also be an overloaded or otherwise failing Cincinnati server.
110110

111111
### ResponseInvalid
112112

113-
The Cincinnati server returned a response that was not valid JSON.
113+
The Cincinnati server returned a response that was not valid JSON or is otherwise corrupted.
114114

115115
This could be caused by a buggy Cincinnati server.
116116
It could also be caused by response corruption, e.g. if the configured `upstream` was in the clear over HTTP or via a man-in-the-middle HTTPS proxy, and an intervening component altered the response in flight.

pkg/autoupdate/autoupdate.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,10 @@ func (ctrl *Controller) sync(ctx context.Context, key string) error {
175175
return nil
176176
}
177177
up := nextUpdate(clusterversion.Status.AvailableUpdates)
178-
clusterversion.Spec.DesiredUpdate = &up
178+
clusterversion.Spec.DesiredUpdate = &v1.Update{
179+
Version: up.Version,
180+
Image: up.Image,
181+
}
179182

180183
_, updated, err := resourceapply.ApplyClusterVersionFromCache(ctx, ctrl.cvLister, ctrl.client.ConfigV1(), clusterversion)
181184
if updated {
@@ -184,11 +187,11 @@ func (ctrl *Controller) sync(ctx context.Context, key string) error {
184187
return err
185188
}
186189

187-
func updateAvail(ups []v1.Update) bool {
190+
func updateAvail(ups []v1.Release) bool {
188191
return len(ups) > 0
189192
}
190193

191-
func nextUpdate(ups []v1.Update) v1.Update {
194+
func nextUpdate(ups []v1.Release) v1.Release {
192195
sorted := ups
193196
sort.Slice(sorted, func(i, j int) bool {
194197
vi := semver.MustParse(sorted[i].Version)

pkg/autoupdate/autoupdate_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,9 @@ func TestNextUpdate(t *testing.T) {
2929
}}
3030
for idx, test := range tests {
3131
t.Run(fmt.Sprintf("test: #%d", idx), func(t *testing.T) {
32-
ups := []v1.Update{}
32+
ups := []v1.Release{}
3333
for _, v := range test.avail {
34-
ups = append(ups, v1.Update{Version: v})
34+
ups = append(ups, v1.Release{Version: v})
3535
}
3636

3737
got := nextUpdate(ups)

pkg/cincinnati/cincinnati.go

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -56,14 +56,15 @@ func (err *Error) Error() string {
5656
return fmt.Sprintf("%s: %s", err.Reason, err.Message)
5757
}
5858

59-
// GetUpdates fetches the next-applicable update payloads from the specified
59+
// GetUpdates fetches the current and next-applicable update payloads from the specified
6060
// upstream Cincinnati stack given the current version and channel. The next-
6161
// applicable updates are determined by downloading the update graph, finding
6262
// the current version within that graph (typically the root node), and then
6363
// finding all of the children. These children are the available updates for
6464
// the current version and their payloads indicate from where the actual update
6565
// image can be downloaded.
66-
func (c Client) GetUpdates(ctx context.Context, uri *url.URL, arch string, channel string, version semver.Version) ([]Update, error) {
66+
func (c Client) GetUpdates(ctx context.Context, uri *url.URL, arch string, channel string, version semver.Version) (Update, []Update, error) {
67+
var current Update
6768
transport := http.Transport{}
6869
// Prepare parametrized cincinnati query.
6970
queryParams := uri.Query()
@@ -76,7 +77,7 @@ func (c Client) GetUpdates(ctx context.Context, uri *url.URL, arch string, chann
7677
// Download the update graph.
7778
req, err := http.NewRequest("GET", uri.String(), nil)
7879
if err != nil {
79-
return nil, &Error{Reason: "InvalidRequest", Message: err.Error(), cause: err}
80+
return current, nil, &Error{Reason: "InvalidRequest", Message: err.Error(), cause: err}
8081
}
8182
req.Header.Add("Accept", GraphMediaType)
8283
if c.tlsConfig != nil {
@@ -92,23 +93,23 @@ func (c Client) GetUpdates(ctx context.Context, uri *url.URL, arch string, chann
9293
defer cancel()
9394
resp, err := client.Do(req.WithContext(timeoutCtx))
9495
if err != nil {
95-
return nil, &Error{Reason: "RemoteFailed", Message: err.Error(), cause: err}
96+
return current, nil, &Error{Reason: "RemoteFailed", Message: err.Error(), cause: err}
9697
}
9798
defer resp.Body.Close()
9899

99100
if resp.StatusCode != http.StatusOK {
100-
return nil, &Error{Reason: "ResponseFailed", Message: fmt.Sprintf("unexpected HTTP status: %s", resp.Status)}
101+
return current, nil, &Error{Reason: "ResponseFailed", Message: fmt.Sprintf("unexpected HTTP status: %s", resp.Status)}
101102
}
102103

103104
// Parse the graph.
104105
body, err := ioutil.ReadAll(resp.Body)
105106
if err != nil {
106-
return nil, &Error{Reason: "ResponseFailed", Message: err.Error(), cause: err}
107+
return current, nil, &Error{Reason: "ResponseFailed", Message: err.Error(), cause: err}
107108
}
108109

109110
var graph graph
110111
if err = json.Unmarshal(body, &graph); err != nil {
111-
return nil, &Error{Reason: "ResponseInvalid", Message: err.Error(), cause: err}
112+
return current, nil, &Error{Reason: "ResponseInvalid", Message: err.Error(), cause: err}
112113
}
113114

114115
// Find the current version within the graph.
@@ -117,12 +118,13 @@ func (c Client) GetUpdates(ctx context.Context, uri *url.URL, arch string, chann
117118
for i, node := range graph.Nodes {
118119
if version.EQ(node.Version) {
119120
currentIdx = i
121+
current = Update(graph.Nodes[i])
120122
found = true
121123
break
122124
}
123125
}
124126
if !found {
125-
return nil, &Error{
127+
return current, nil, &Error{
126128
Reason: "VersionNotFound",
127129
Message: fmt.Sprintf("currently installed version %s not found in the %q channel", version, channel),
128130
}
@@ -141,7 +143,7 @@ func (c Client) GetUpdates(ctx context.Context, uri *url.URL, arch string, chann
141143
updates = append(updates, Update(graph.Nodes[i]))
142144
}
143145

144-
return updates, nil
146+
return current, updates, nil
145147
}
146148

147149
type graph struct {
@@ -150,8 +152,9 @@ type graph struct {
150152
}
151153

152154
type node struct {
153-
Version semver.Version `json:"version"`
154-
Image string `json:"payload"`
155+
Version semver.Version `json:"version"`
156+
Image string `json:"payload"`
157+
Metadata map[string]string `json:"metadata,omitempty"`
155158
}
156159

157160
type edge struct {

pkg/cincinnati/cincinnati_test.go

Lines changed: 31 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -25,26 +25,30 @@ func TestGetUpdates(t *testing.T) {
2525
version string
2626

2727
expectedQuery string
28+
current Update
2829
available []Update
2930
err string
3031
}{{
3132
name: "one update available",
3233
version: "4.0.0-4",
3334
expectedQuery: "arch=test-arch&channel=test-channel&id=01234567-0123-0123-0123-0123456789ab&version=4.0.0-4",
35+
current: Update{Version: semver.MustParse("4.0.0-4"), Image: "quay.io/openshift-release-dev/ocp-release:4.0.0-4"},
3436
available: []Update{
35-
{semver.MustParse("4.0.0-5"), "quay.io/openshift-release-dev/ocp-release:4.0.0-5"},
37+
{Version: semver.MustParse("4.0.0-5"), Image: "quay.io/openshift-release-dev/ocp-release:4.0.0-5"},
3638
},
3739
}, {
3840
name: "two updates available",
3941
version: "4.0.0-5",
4042
expectedQuery: "arch=test-arch&channel=test-channel&id=01234567-0123-0123-0123-0123456789ab&version=4.0.0-5",
43+
current: Update{Version: semver.MustParse("4.0.0-5"), Image: "quay.io/openshift-release-dev/ocp-release:4.0.0-5"},
4144
available: []Update{
42-
{semver.MustParse("4.0.0-6"), "quay.io/openshift-release-dev/ocp-release:4.0.0-6"},
43-
{semver.MustParse("4.0.0-6+2"), "quay.io/openshift-release-dev/ocp-release:4.0.0-6+2"},
45+
{Version: semver.MustParse("4.0.0-6"), Image: "quay.io/openshift-release-dev/ocp-release:4.0.0-6"},
46+
{Version: semver.MustParse("4.0.0-6+2"), Image: "quay.io/openshift-release-dev/ocp-release:4.0.0-6+2"},
4447
},
4548
}, {
4649
name: "no updates available",
4750
version: "4.0.0-0.okd-0",
51+
current: Update{Version: semver.MustParse("4.0.0-0.okd-0"), Image: "quay.io/openshift-release-dev/ocp-release:4.0.0-0.okd-0"},
4852
expectedQuery: "arch=test-arch&channel=test-channel&id=01234567-0123-0123-0123-0123456789ab&version=4.0.0-0.okd-0",
4953
}, {
5054
name: "unknown version",
@@ -79,38 +83,31 @@ func TestGetUpdates(t *testing.T) {
7983
"nodes": [
8084
{
8185
"version": "4.0.0-4",
82-
"payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-4",
83-
"metadata": {}
86+
"payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-4"
8487
},
8588
{
8689
"version": "4.0.0-5",
87-
"payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-5",
88-
"metadata": {}
90+
"payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-5"
8991
},
9092
{
9193
"version": "4.0.0-6",
92-
"payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-6",
93-
"metadata": {}
94+
"payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-6"
9495
},
9596
{
9697
"version": "4.0.0-6+2",
97-
"payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-6+2",
98-
"metadata": {}
98+
"payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-6+2"
9999
},
100100
{
101101
"version": "4.0.0-0.okd-0",
102-
"payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-0.okd-0",
103-
"metadata": {}
102+
"payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-0.okd-0"
104103
},
105104
{
106105
"version": "4.0.0-0.2",
107-
"payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-0.2",
108-
"metadata": {}
106+
"payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-0.2"
109107
},
110108
{
111109
"version": "4.0.0-0.3",
112-
"payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-0.3",
113-
"metadata": {}
110+
"payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-0.3"
114111
}
115112
],
116113
"edges": [[0,1],[1,2],[1,3],[5,6]]
@@ -133,13 +130,16 @@ func TestGetUpdates(t *testing.T) {
133130
t.Fatal(err)
134131
}
135132

136-
updates, err := c.GetUpdates(context.Background(), uri, arch, channelName, semver.MustParse(test.version))
133+
current, updates, err := c.GetUpdates(context.Background(), uri, arch, channelName, semver.MustParse(test.version))
137134
if test.err == "" {
138135
if err != nil {
139136
t.Fatalf("expected nil error, got: %v", err)
140137
}
138+
if !reflect.DeepEqual(current, test.current) {
139+
t.Fatalf("expected current %v, got: %v", test.current, current)
140+
}
141141
if !reflect.DeepEqual(updates, test.available) {
142-
t.Fatalf("expected %v, got: %v", test.available, updates)
142+
t.Fatalf("expected updates %v, got: %v", test.available, updates)
143143
}
144144
} else {
145145
if err == nil || err.Error() != test.err {
@@ -181,7 +181,11 @@ func Test_nodeUnmarshalJSON(t *testing.T) {
181181
"metadata": {}
182182
}`),
183183

184-
exp: node{semver.MustParse("4.0.0-5"), "quay.io/openshift-release-dev/ocp-release:4.0.0-5"},
184+
exp: node{
185+
Version: semver.MustParse("4.0.0-5"),
186+
Image: "quay.io/openshift-release-dev/ocp-release:4.0.0-5",
187+
Metadata: map[string]string{},
188+
},
185189
}, {
186190
raw: []byte(`{
187191
"version": "4.0.0-0.1",
@@ -190,7 +194,13 @@ func Test_nodeUnmarshalJSON(t *testing.T) {
190194
"description": "This is the beta1 image based on the 4.0.0-0.nightly-2019-01-15-010905 build"
191195
}
192196
}`),
193-
exp: node{semver.MustParse("4.0.0-0.1"), "quay.io/openshift-release-dev/ocp-release:4.0.0-0.1"},
197+
exp: node{
198+
Version: semver.MustParse("4.0.0-0.1"),
199+
Image: "quay.io/openshift-release-dev/ocp-release:4.0.0-0.1",
200+
Metadata: map[string]string{
201+
"description": "This is the beta1 image based on the 4.0.0-0.nightly-2019-01-15-010905 build",
202+
},
203+
},
194204
}, {
195205
raw: []byte(`{
196206
"version": "v4.0.0-0.1",

0 commit comments

Comments
 (0)