Skip to content

Allow excluding manifests from CVO payload #252

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

Merged
openshift-merge-robot merged 1 commit into from
Nov 14, 2019
Merged

Allow excluding manifests from CVO payload #252

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
8 changes: 8 additions & 0 deletions pkg/cvo/cvo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,10 @@ type Operator struct {
// lastAtLock guards access to controller memory about the sync loop
lastAtLock sync.Mutex
lastResourceVersion int64

// exclude is an optional identifier used to exclude certain manifests
// via annotation
exclude string
}

// New returns a new cluster version operator.
Expand All @@ -156,6 +160,7 @@ func New(
client clientset.Interface,
kubeClient kubernetes.Interface,
enableMetrics bool,
exclude string,
) *Operator {
eventBroadcaster := record.NewBroadcaster()
eventBroadcaster.StartLogging(klog.Infof)
Expand All @@ -181,6 +186,8 @@ func New(
queue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "clusterversion"),
availableUpdatesQueue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "availableupdates"),
upgradeableQueue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "upgradeable"),

exclude: exclude,
}

cvInformer.Informer().AddEventHandler(optr.eventHandler())
Expand Down Expand Up @@ -246,6 +253,7 @@ func (optr *Operator) InitializeFromPayload(restConfig *rest.Config, burstRestCo
Factor: 1.3,
Steps: 3,
},
optr.exclude,
)

return nil
Expand Down
1 change: 1 addition & 0 deletions pkg/cvo/cvo_scenarios_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ func setupCVOTest(payloadDir string) (*Operator, map[string]runtime.Object, *fak
wait.Backoff{
Steps: 1,
},
"",
)
o.configSync = worker

Expand Down
31 changes: 21 additions & 10 deletions pkg/cvo/sync_worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,11 +126,11 @@ func (w SyncWorkerStatus) DeepCopy() *SyncWorkerStatus {
// syncOnce() returns nil -> Reconciling
//
type SyncWorker struct {
backoff wait.Backoff
retriever PayloadRetriever
builder payload.ResourceBuilder
preconditions precondition.List
reconciling bool
backoff wait.Backoff
retriever PayloadRetriever
builder payload.ResourceBuilder
preconditions precondition.List
reconciling bool

// minimumReconcileInterval is the minimum time between reconcile attempts, and is
// used to define the maximum backoff interval when syncOnce() returns an error.
Expand All @@ -148,11 +148,16 @@ type SyncWorker struct {

// updated by the run method only
payload *payload.Update

// exclude is an identifier used to determine which
// manifests should be excluded based on an annotation
// of the form exclude.release.openshift.io/<identifier>=true
exclude string
Copy link
Copy Markdown
Member

@wking wking Nov 13, 2019

Choose a reason for hiding this comment

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

Do we want a single string, here or a []string of identifiers to exclude? Related to this thread.

Copy link
Copy Markdown
Contributor Author

@csrwng csrwng Nov 15, 2019

Choose a reason for hiding this comment

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

@wking the idea is that the identifier would be the equivalent of a single profile. If you have a different exclusion profile, you should use a different identifier. Manifests in the individual operators can contain exclusion annotations for multiple profiles.

}

// NewSyncWorker initializes a ConfigSyncWorker that will retrieve payloads to disk, apply them via builder
// to a server, and obey limits about how often to reconcile or retry on errors.
func NewSyncWorker(retriever PayloadRetriever, builder payload.ResourceBuilder, reconcileInterval time.Duration, backoff wait.Backoff) *SyncWorker {
func NewSyncWorker(retriever PayloadRetriever, builder payload.ResourceBuilder, reconcileInterval time.Duration, backoff wait.Backoff, exclude string) *SyncWorker {
return &SyncWorker{
retriever: retriever,
builder: builder,
Expand All @@ -165,14 +170,16 @@ func NewSyncWorker(retriever PayloadRetriever, builder payload.ResourceBuilder,
// Status() or use the result of calling Update() instead because the channel can be out of date
// if the reader is not fast enough.
report: make(chan SyncWorkerStatus, 500),

exclude: exclude,
}
}

// NewSyncWorkerWithPreconditions initializes a ConfigSyncWorker that will retrieve payloads to disk, apply them via builder
// to a server, and obey limits about how often to reconcile or retry on errors.
// It allows providing preconditions for loading payload.
func NewSyncWorkerWithPreconditions(retriever PayloadRetriever, builder payload.ResourceBuilder, preconditions precondition.List, reconcileInterval time.Duration, backoff wait.Backoff) *SyncWorker {
worker := NewSyncWorker(retriever, builder, reconcileInterval, backoff)
func NewSyncWorkerWithPreconditions(retriever PayloadRetriever, builder payload.ResourceBuilder, preconditions precondition.List, reconcileInterval time.Duration, backoff wait.Backoff, exclude string) *SyncWorker {
worker := NewSyncWorker(retriever, builder, reconcileInterval, backoff, exclude)
worker.preconditions = preconditions
return worker
}
Expand Down Expand Up @@ -614,7 +621,7 @@ func (w *SyncWorker) apply(ctx context.Context, payloadUpdate *payload.Update, w
klog.V(4).Infof("Running sync for %s", task)
klog.V(5).Infof("Manifest: %s", string(task.Manifest.Raw))

ov, ok := getOverrideForManifest(work.Overrides, task.Manifest)
ov, ok := getOverrideForManifest(work.Overrides, w.exclude, task.Manifest)
if ok && ov.Unmanaged {
klog.V(4).Infof("Skipping %s as unmanaged", task)
continue
Expand Down Expand Up @@ -906,7 +913,7 @@ func newMultipleError(errs []error) error {
}

// getOverrideForManifest returns the override and true when override exists for manifest.
func getOverrideForManifest(overrides []configv1.ComponentOverride, manifest *lib.Manifest) (configv1.ComponentOverride, bool) {
func getOverrideForManifest(overrides []configv1.ComponentOverride, excludeIdentifier string, manifest *lib.Manifest) (configv1.ComponentOverride, bool) {
for idx, ov := range overrides {
kind, namespace, name := manifest.GVK.Kind, manifest.Object().GetNamespace(), manifest.Object().GetName()
if ov.Kind == kind &&
Expand All @@ -915,6 +922,10 @@ func getOverrideForManifest(overrides []configv1.ComponentOverride, manifest *li
return overrides[idx], true
}
}
excludeAnnotation := fmt.Sprintf("exclude.release.openshift.io/%s", excludeIdentifier)
if annotations := manifest.Object().GetAnnotations(); annotations != nil && annotations[excludeAnnotation] == "true" {
return configv1.ComponentOverride{Unmanaged: true}, true
}
return configv1.ComponentOverride{}, false
}

Expand Down
10 changes: 10 additions & 0 deletions pkg/start/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ type Options struct {
EnableAutoUpdate bool
EnableDefaultClusterVersion bool

// Exclude is used to determine whether to exclude
// certain manifests based on an annotation:
// exclude.release.openshift.io/<identifier>=true
Exclude string

// for testing only
Name string
Namespace string
Expand Down Expand Up @@ -87,6 +92,7 @@ func NewOptions() *Options {
PayloadOverride: os.Getenv("PAYLOAD_OVERRIDE"),
ResyncInterval: minResyncPeriod,
EnableMetrics: true,
Exclude: os.Getenv("EXCLUDE_MANIFESTS"),
Copy link
Copy Markdown
Contributor

@smarterclayton smarterclayton Nov 13, 2019

Choose a reason for hiding this comment

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

When exclude is set we should log this.

Excluding manifests for %q

in the main logs so we can see it if we have bugs.

Copy link
Copy Markdown
Contributor Author

@csrwng csrwng Nov 13, 2019

Choose a reason for hiding this comment

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

done

}
}

Expand All @@ -100,6 +106,9 @@ func (o *Options) Run() error {
if len(o.PayloadOverride) > 0 {
klog.Warningf("Using an override payload directory for testing only: %s", o.PayloadOverride)
}
if len(o.Exclude) > 0 {
klog.Infof("Excluding manifests for %q", o.Exclude)
}

// initialize the core objects
cb, err := newClientBuilder(o.Kubeconfig)
Expand Down Expand Up @@ -337,6 +346,7 @@ func (o *Options) NewControllerContext(cb *ClientBuilder) *Context {
cb.ClientOrDie(o.Namespace),
cb.KubeClientOrDie(o.Namespace, useProtobuf),
o.EnableMetrics,
o.Exclude,
),
}
if o.EnableAutoUpdate {
Expand Down
8 changes: 4 additions & 4 deletions pkg/start/start_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ func TestIntegrationCVO_initializeAndUpgrade(t *testing.T) {
options.EnableMetrics = false
controllers := options.NewControllerContext(cb)

worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil), 5*time.Second, wait.Backoff{Steps: 3})
worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil), 5*time.Second, wait.Backoff{Steps: 3}, "")
controllers.CVO.SetSyncWorkerForTesting(worker)

ctx, cancel := context.WithCancel(context.Background())
Expand Down Expand Up @@ -394,7 +394,7 @@ func TestIntegrationCVO_initializeAndHandleError(t *testing.T) {
options.ResyncInterval = 3 * time.Second
controllers := options.NewControllerContext(cb)

worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil), 5*time.Second, wait.Backoff{Duration: time.Second, Factor: 1.2})
worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil), 5*time.Second, wait.Backoff{Duration: time.Second, Factor: 1.2}, "")
controllers.CVO.SetSyncWorkerForTesting(worker)

ctx, cancel := context.WithCancel(context.Background())
Expand Down Expand Up @@ -500,7 +500,7 @@ func TestIntegrationCVO_gracefulStepDown(t *testing.T) {
options.EnableMetrics = false
controllers := options.NewControllerContext(cb)

worker := cvo.NewSyncWorker(&mapPayloadRetriever{}, cvo.NewResourceBuilder(cfg, cfg, nil), 5*time.Second, wait.Backoff{Steps: 3})
worker := cvo.NewSyncWorker(&mapPayloadRetriever{}, cvo.NewResourceBuilder(cfg, cfg, nil), 5*time.Second, wait.Backoff{Steps: 3}, "")
controllers.CVO.SetSyncWorkerForTesting(worker)

lock, err := createResourceLock(cb, ns, ns)
Expand Down Expand Up @@ -673,7 +673,7 @@ metadata:
t.Fatal(err)
}

worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil), 5*time.Second, wait.Backoff{Steps: 3})
worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil), 5*time.Second, wait.Backoff{Steps: 3}, "")
controllers.CVO.SetSyncWorkerForTesting(worker)

ctx, cancel := context.WithCancel(context.Background())
Expand Down