Skip to content

Commit c410434

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #1184 from petr-muller/cvo-options-refactor
OTA-1531: [1/x] cvo: refactor option processing
2 parents 55b7593 + 40257c9 commit c410434

File tree

3 files changed

+61
-52
lines changed

3 files changed

+61
-52
lines changed

‎cmd/cluster-version-operator/start.go‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package main
22

33
import (
44
"context"
5+
56
"github.com/spf13/cobra"
67
"k8s.io/klog/v2"
78

@@ -15,10 +16,12 @@ func init() {
1516
Use: "start",
1617
Short: "Starts Cluster Version Operator",
1718
Long: "",
18-
Run: func(cmd *cobra.Command, args []string) {
19+
PreRunE: func(_ *cobra.Command, _ []string) error {
1920
// To help debugging, immediately log version
2021
klog.Info(version.String)
21-
22+
return opts.ValidateAndComplete()
23+
},
24+
Run: func(_ *cobra.Command, _ []string) {
2225
if err := opts.Run(context.Background()); err != nil {
2326
klog.Fatalf("error: %v", err)
2427
}

‎pkg/start/start.go‎

Lines changed: 41 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2020
"k8s.io/apimachinery/pkg/fields"
2121
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
22+
"k8s.io/apimachinery/pkg/util/sets"
2223
"k8s.io/apimachinery/pkg/util/wait"
2324
"k8s.io/client-go/informers"
2425
"k8s.io/client-go/kubernetes"
@@ -132,7 +133,7 @@ func NewOptions() *Options {
132133
}
133134
}
134135

135-
func (o *Options) Run(ctx context.Context) error {
136+
func (o *Options) ValidateAndComplete() error {
136137
if o.NodeName == "" {
137138
return fmt.Errorf("node-name is required")
138139
}
@@ -152,25 +153,29 @@ func (o *Options) Run(ctx context.Context) error {
152153
(o.PromQLTarget.KubeSvc.Namespace == "" || o.PromQLTarget.KubeSvc.Name == "") {
153154
return fmt.Errorf("--use-dns-for-services is disabled, so --metrics-service and --metrics-namespace must be set")
154155
}
155-
if len(o.PayloadOverride) > 0 {
156-
klog.Warningf("Using an override payload directory for testing only: %s", o.PayloadOverride)
157-
}
158-
if len(o.Exclude) > 0 {
159-
klog.Infof("Excluding manifests for %q", o.Exclude)
160-
}
161-
alwaysEnableCaps, unknownCaps := parseAlwaysEnableCapabilities(o.AlwaysEnableCapabilities)
162-
if len(unknownCaps) > 0 {
163-
return fmt.Errorf("--always-enable-capabilities was set with unknown capabilities: %v", unknownCaps)
156+
157+
if parsed, err := url.Parse(o.PrometheusURLString); err != nil {
158+
return fmt.Errorf("error parsing promql url: %v", err)
159+
} else {
160+
o.PromQLTarget.URL = parsed
164161
}
165162

166163
// Inject the cluster ID into PromQL queries in HyperShift
167164
o.InjectClusterIdIntoPromQL = o.HyperShift
168165

169-
// parse the prometheus url
170-
var err error
171-
o.PromQLTarget.URL, err = url.Parse(o.PrometheusURLString)
172-
if err != nil {
173-
return fmt.Errorf("error parsing promql url: %v", err)
166+
if err := validateCapabilities(o.AlwaysEnableCapabilities); err != nil {
167+
return fmt.Errorf("--always-enable-capabilities: %w", err)
168+
}
169+
170+
return nil
171+
}
172+
173+
func (o *Options) Run(ctx context.Context) error {
174+
if len(o.PayloadOverride) > 0 {
175+
klog.Warningf("Using an override payload directory for testing only: %s", o.PayloadOverride)
176+
}
177+
if len(o.Exclude) > 0 {
178+
klog.Infof("Excluding manifests for %q", o.Exclude)
174179
}
175180

176181
// initialize the core objects
@@ -185,7 +190,7 @@ func (o *Options) Run(ctx context.Context) error {
185190
}
186191

187192
// initialize the controllers and attempt to load the payload information
188-
controllerCtx, err := o.NewControllerContext(cb, alwaysEnableCaps)
193+
controllerCtx, err := o.NewControllerContext(cb)
189194
if err != nil {
190195
return err
191196
}
@@ -470,7 +475,7 @@ type Context struct {
470475

471476
// NewControllerContext initializes the default Context for the current Options. It does
472477
// not start any background processes.
473-
func (o *Options) NewControllerContext(cb *ClientBuilder, alwaysEnableCapabilities []configv1.ClusterVersionCapability) (*Context, error) {
478+
func (o *Options) NewControllerContext(cb *ClientBuilder) (*Context, error) {
474479
client := cb.ClientOrDie("shared-informer")
475480
kubeClient := cb.KubeClientOrDie(internal.ConfigNamespace, useProtobuf)
476481
operatorClient := cb.OperatorClientOrDie("operator-client")
@@ -490,6 +495,7 @@ func (o *Options) NewControllerContext(cb *ClientBuilder, alwaysEnableCapabiliti
490495

491496
cvoKubeClient := cb.KubeClientOrDie(o.Namespace, useProtobuf)
492497
o.PromQLTarget.KubeClient = cvoKubeClient
498+
493499
cvo, err := cvo.New(
494500
o.NodeName,
495501
o.Namespace, o.Name,
@@ -511,7 +517,7 @@ func (o *Options) NewControllerContext(cb *ClientBuilder, alwaysEnableCapabiliti
511517
o.PromQLTarget,
512518
o.InjectClusterIdIntoPromQL,
513519
o.UpdateService,
514-
alwaysEnableCapabilities,
520+
stringsToCapabilities(o.AlwaysEnableCapabilities),
515521
)
516522
if err != nil {
517523
return nil, err
@@ -618,25 +624,22 @@ func (c *Context) InitializeFromPayload(ctx context.Context, restConfig *rest.Co
618624
return nil
619625
}
620626

621-
// parseAlwaysEnableCapabilities parses the string list of capabilities
622-
// into two lists of configv1.ClusterVersionCapability: known and unknown.
623-
func parseAlwaysEnableCapabilities(caps []string) ([]configv1.ClusterVersionCapability, []configv1.ClusterVersionCapability) {
624-
var (
625-
knownCaps []configv1.ClusterVersionCapability
626-
unknownCaps []configv1.ClusterVersionCapability
627-
)
628-
for _, c := range caps {
629-
known := false
630-
for _, kc := range configv1.KnownClusterVersionCapabilities {
631-
if configv1.ClusterVersionCapability(c) == kc {
632-
knownCaps = append(knownCaps, kc)
633-
known = true
634-
break
635-
}
636-
}
637-
if !known {
638-
unknownCaps = append(unknownCaps, configv1.ClusterVersionCapability(c))
639-
}
627+
func stringsToCapabilities(names []string) []configv1.ClusterVersionCapability {
628+
caps := make([]configv1.ClusterVersionCapability, len(names))
629+
for i, c := range names {
630+
caps[i] = configv1.ClusterVersionCapability(c)
631+
}
632+
return caps
633+
}
634+
635+
func validateCapabilities(caps []string) error {
636+
unknown := sets.New(caps...)
637+
for _, kc := range configv1.KnownClusterVersionCapabilities {
638+
unknown.Delete(string(kc))
639+
}
640+
641+
if len(unknown) > 0 {
642+
return fmt.Errorf("unknown capabilities: %s", sets.List(unknown))
640643
}
641-
return knownCaps, unknownCaps
644+
return nil
642645
}

‎pkg/start/start_integration_test.go‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -184,15 +184,16 @@ func TestIntegrationCVO_initializeAndUpgrade(t *testing.T) {
184184
options.ReleaseImage = payloadImage1
185185
options.PayloadOverride = filepath.Join(dir, "0.0.1")
186186
options.leaderElection = getLeaderElectionConfig(ctx, cfg)
187-
alwaysEnableCapabilities := []configv1.ClusterVersionCapability{
188-
configv1.ClusterVersionCapabilityIngress,
187+
options.AlwaysEnableCapabilities = []string{string(configv1.ClusterVersionCapabilityIngress)}
188+
if err := options.ValidateAndComplete(); err != nil {
189+
t.Fatalf("incorrectly initialized options: %v", err)
189190
}
190-
controllers, err := options.NewControllerContext(cb, alwaysEnableCapabilities)
191+
controllers, err := options.NewControllerContext(cb)
191192
if err != nil {
192193
t.Fatal(err)
193194
}
194195

195-
worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil, nil), 5*time.Second, wait.Backoff{Steps: 3}, "", "", record.NewFakeRecorder(100), payload.DefaultClusterProfile, alwaysEnableCapabilities)
196+
worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil, nil), 5*time.Second, wait.Backoff{Steps: 3}, "", "", record.NewFakeRecorder(100), payload.DefaultClusterProfile, stringsToCapabilities(options.AlwaysEnableCapabilities))
196197
controllers.CVO.SetSyncWorkerForTesting(worker)
197198

198199
lock, err := createResourceLock(cb, options.Namespace, options.Name)
@@ -318,15 +319,16 @@ func TestIntegrationCVO_gracefulStepDown(t *testing.T) {
318319
options.ReleaseImage = payloadImage1
319320
options.PayloadOverride = filepath.Join(dir, "0.0.1")
320321
options.leaderElection = getLeaderElectionConfig(ctx, cfg)
321-
alwaysEnableCapabilities := []configv1.ClusterVersionCapability{
322-
configv1.ClusterVersionCapabilityIngress,
322+
options.AlwaysEnableCapabilities = []string{string(configv1.ClusterVersionCapabilityIngress)}
323+
if err := options.ValidateAndComplete(); err != nil {
324+
t.Fatalf("incorrectly initialized options: %v", err)
323325
}
324-
controllers, err := options.NewControllerContext(cb, alwaysEnableCapabilities)
326+
controllers, err := options.NewControllerContext(cb)
325327
if err != nil {
326328
t.Fatal(err)
327329
}
328330

329-
worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil, nil), 5*time.Second, wait.Backoff{Steps: 3}, "", "", record.NewFakeRecorder(100), payload.DefaultClusterProfile, alwaysEnableCapabilities)
331+
worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil, nil), 5*time.Second, wait.Backoff{Steps: 3}, "", "", record.NewFakeRecorder(100), payload.DefaultClusterProfile, stringsToCapabilities(options.AlwaysEnableCapabilities))
330332
controllers.CVO.SetSyncWorkerForTesting(worker)
331333

332334
lock, err := createResourceLock(cb, options.Namespace, options.Name)
@@ -514,15 +516,16 @@ metadata:
514516
options.ReleaseImage = payloadImage1
515517
options.PayloadOverride = payloadDir
516518
options.leaderElection = getLeaderElectionConfig(ctx, cfg)
517-
alwaysEnableCapabilities := []configv1.ClusterVersionCapability{
518-
configv1.ClusterVersionCapabilityIngress,
519+
options.AlwaysEnableCapabilities = []string{string(configv1.ClusterVersionCapabilityIngress)}
520+
if err := options.ValidateAndComplete(); err != nil {
521+
t.Fatalf("incorrectly initialized options: %v", err)
519522
}
520-
controllers, err := options.NewControllerContext(cb, alwaysEnableCapabilities)
523+
controllers, err := options.NewControllerContext(cb)
521524
if err != nil {
522525
t.Fatal(err)
523526
}
524527

525-
worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil, nil), 5*time.Second, wait.Backoff{Steps: 3}, "", "", record.NewFakeRecorder(100), payload.DefaultClusterProfile, alwaysEnableCapabilities)
528+
worker := cvo.NewSyncWorker(retriever, cvo.NewResourceBuilder(cfg, cfg, nil, nil), 5*time.Second, wait.Backoff{Steps: 3}, "", "", record.NewFakeRecorder(100), payload.DefaultClusterProfile, stringsToCapabilities(options.AlwaysEnableCapabilities))
526529
controllers.CVO.SetSyncWorkerForTesting(worker)
527530

528531
lock, err := createResourceLock(cb, options.Namespace, options.Name)

0 commit comments

Comments
 (0)