Skip to content

pkg/cincinnati: scope HTTP request to a context #83

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.

Sign up for GitHub

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

Closed
lucab wants to merge 1 commit into from
Closed

pkg/cincinnati: scope HTTP request to a context #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions pkg/cincinnati/cincinnati.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package cincinnati

import (
"context"
"encoding/json"
"fmt"
"io/ioutil"
Expand Down Expand Up @@ -38,7 +39,7 @@ type Update node
// finding all of the children. These children are the available updates for
// the current version and their payloads indicate from where the actual update
// image can be downloaded.
func (c Client) GetUpdates(upstream string, channel string, version semver.Version) ([]Update, error) {
func (c Client) GetUpdates(ctx context.Context, upstream string, channel string, version semver.Version) ([]Update, error) {
// Prepare parametrized cincinnati query.
cincinnatiURL, err := url.Parse(upstream)
if err != nil {
Expand All @@ -58,7 +59,7 @@ func (c Client) GetUpdates(upstream string, channel string, version semver.Versi
req.Header.Add("Accept", GraphMediaType)

client := http.Client{}
resp, err := client.Do(req)
resp, err := client.Do(req.WithContext(ctx))
if err != nil {
return nil, err
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/cincinnati/cincinnati_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package cincinnati

import (
"context"
"encoding/json"
"fmt"
"net/http"
Expand Down Expand Up @@ -123,7 +124,7 @@ func TestGetUpdates(t *testing.T) {

c := NewClient(clientID)

updates, err := c.GetUpdates(ts.URL, channelName, semver.MustParse(test.version))
updates, err := c.GetUpdates(context.TODO(), ts.URL, channelName, semver.MustParse(test.version))
if test.err == "" {
if err != nil {
t.Fatalf("expected nil error, got: %v", err)
Expand Down
13 changes: 7 additions & 6 deletions pkg/cvo/availableupdates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package cvo

import (
"context"
"fmt"
"time"

Expand All @@ -20,7 +21,7 @@ import (
// syncAvailableUpdates attempts to retrieve the latest updates and update the status of the ClusterVersion
// object. It will set the RetrievedUpdates condition. Updates are only checked if it has been more than
// the minimumUpdateCheckInterval since the last check.
func (optr *Operator) syncAvailableUpdates(config *configv1.ClusterVersion) error {
func (optr *Operator) syncAvailableUpdates(ctx context.Context, config *configv1.ClusterVersion) error {
usedDefaultUpstream := false
upstream := string(config.Spec.Upstream)
if len(upstream) == 0 {
Expand All @@ -36,7 +37,7 @@ func (optr *Operator) syncAvailableUpdates(config *configv1.ClusterVersion) erro
return nil
}

updates, condition := calculateAvailableUpdatesStatus(string(config.Spec.ClusterID), upstream, channel, optr.releaseVersion)
updates, condition := calculateAvailableUpdatesStatus(ctx, string(config.Spec.ClusterID), upstream, channel, optr.releaseVersion)

if usedDefaultUpstream {
upstream = ""
Expand Down Expand Up @@ -103,7 +104,7 @@ func (optr *Operator) getAvailableUpdates() *availableUpdates {
return optr.availableUpdates
}

func calculateAvailableUpdatesStatus(clusterID, upstream, channel, version string) ([]configv1.Update, configv1.ClusterOperatorStatusCondition) {
func calculateAvailableUpdatesStatus(ctx context.Context, clusterID, upstream, channel, version string) ([]configv1.Update, configv1.ClusterOperatorStatusCondition) {
if len(upstream) == 0 {
return nil, configv1.ClusterOperatorStatusCondition{
Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse, Reason: "NoUpstream",
Expand All @@ -127,7 +128,7 @@ func calculateAvailableUpdatesStatus(clusterID, upstream, channel, version strin
}
}

updates, err := checkForUpdate(clusterID, upstream, channel, currentVersion)
Copy link
Contributor

@abhinavdahiya abhinavdahiya Jan 15, 2019

Choose a reason for hiding this comment

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

I would assume this is the only place that needs the ctx, why do any body up the chain care about ctx for checking updates?

Copy link
Contributor Author

@lucab lucab Jan 15, 2019

Choose a reason for hiding this comment

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

I can cut the chain at any arbitrary point that you prefer, but please do note that the topmost Until layer already takes a stop channel, but doesn't propagate that down. I'd rather suggest to leave the TODO here and later fix the caller to unify the cancellation chain.

Copy link
Contributor Author

@lucab lucab Jan 29, 2019

Choose a reason for hiding this comment

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

FYI I landed a fix in k8s master to propagate the context through Until: kubernetes/kubernetes#72928

updates, err := checkForUpdate(ctx, clusterID, upstream, channel, currentVersion)
if err != nil {
glog.V(2).Infof("Upstream server %s could not return available updates: %v", upstream, err)
return nil, configv1.ClusterOperatorStatusCondition{
Expand All @@ -152,13 +153,13 @@ func calculateAvailableUpdatesStatus(clusterID, upstream, channel, version strin
}
}

func checkForUpdate(clusterID, upstream, channel string, currentVersion semver.Version) ([]cincinnati.Update, error) {
func checkForUpdate(ctx context.Context, clusterID, upstream, channel string, currentVersion semver.Version) ([]cincinnati.Update, error) {
uuid, err := uuid.Parse(string(clusterID))
if err != nil {
return nil, err
}
if len(upstream) == 0 {
return nil, fmt.Errorf("no upstream URL set for cluster version")
}
return cincinnati.NewClient(uuid).GetUpdates(upstream, channel, currentVersion)
return cincinnati.NewClient(uuid).GetUpdates(ctx, upstream, channel, currentVersion)
}
5 changes: 3 additions & 2 deletions pkg/cvo/cvo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package cvo

import (
"context"
"fmt"
"strconv"
"sync"
Expand Down Expand Up @@ -361,8 +362,8 @@ func (optr *Operator) availableUpdatesSync(key string) error {
if errs := validation.ValidateClusterVersion(config); len(errs) > 0 {
return nil
}

return optr.syncAvailableUpdates(config)
ctx := context.TODO()
return optr.syncAvailableUpdates(ctx, config)
}

// isOlderThanLastUpdate returns true if the cluster version is older than
Expand Down