Skip to content
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

remove disabledfeatures from master config #19070

Merged
merged 2 commits into from
Mar 26, 2018
Merged

remove disabledfeatures from master config #19070

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dc804f6
generated
deads2k Mar 22, 2018
f14a3a5
remove disabledfeatures from master config
deads2k Mar 22, 2018
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
4 changes: 0 additions & 4 deletions hack/test-cmd.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,10 +77,6 @@ os::cmd::expect_success_and_not_text "KUBECONFIG='${MASTER_CONFIG_DIR}/admin.kub
os::cmd::expect_success_and_not_text "KUBECONFIG='' oc version" "Missing or incomplete configuration info"
os::test::junit::declare_suite_end

os::test::junit::declare_suite_start "cmd/config"
os::cmd::expect_success_and_text "cat ${MASTER_CONFIG_DIR}/master-config.yaml" 'disabledFeatures: null'
os::test::junit::declare_suite_end

os::start::master

# profile the cli commands
Expand Down
5 changes: 0 additions & 5 deletions pkg/cmd/openshift-apiserver/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package openshift_apiserver

import (
"strings"

"github.com/golang/glog"

kerrors "k8s.io/apimachinery/pkg/api/errors"
Expand Down Expand Up @@ -55,9 +53,6 @@ func RunOpenShiftAPIServer(masterConfig *configapi.MasterConfig) error {

glog.Infof("Starting master on %s (%s)", masterConfig.ServingInfo.BindAddress, version.Get().String())
glog.Infof("Public master address is %s", masterConfig.MasterPublicURL)
if len(masterConfig.DisabledFeatures) > 0 {
glog.V(4).Infof("Disabled features: %s", strings.Join(masterConfig.DisabledFeatures, ", "))
}
imageTemplate := variable.NewDefaultImageTemplate()
imageTemplate.Format = masterConfig.ImageConfig.Format
imageTemplate.Latest = masterConfig.ImageConfig.Latest
Expand Down
11 changes: 1 addition & 10 deletions pkg/cmd/openshift-controller-manager/controller/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ type OpenshiftControllerConfig struct {
DeployerControllerConfig DeployerControllerConfig
DeploymentConfigControllerConfig DeploymentConfigControllerConfig

ImageTriggerControllerConfig ImageTriggerControllerConfig
ImageSignatureImportControllerConfig ImageSignatureImportControllerConfig
ImageImportControllerConfig ImageImportControllerConfig

Expand Down Expand Up @@ -90,7 +89,7 @@ func (c *OpenshiftControllerConfig) GetControllerInitializers() (map[string]Init
ret["openshift.io/deployer"] = c.DeployerControllerConfig.RunController
ret["openshift.io/deploymentconfig"] = c.DeploymentConfigControllerConfig.RunController

ret["openshift.io/image-trigger"] = c.ImageTriggerControllerConfig.RunController
ret["openshift.io/image-trigger"] = RunImageTriggerController
ret["openshift.io/image-import"] = c.ImageImportControllerConfig.RunController
ret["openshift.io/image-signature-import"] = c.ImageSignatureImportControllerConfig.RunController

Expand Down Expand Up @@ -146,14 +145,6 @@ func BuildOpenshiftControllerConfig(options configapi.MasterConfig) (*OpenshiftC
Codec: annotationCodec,
}

ret.ImageTriggerControllerConfig = ImageTriggerControllerConfig{
HasBuilderEnabled: options.DisabledFeatures.Has(configapi.FeatureBuilder),
// TODO: make these consts in configapi
HasDeploymentsEnabled: options.DisabledFeatures.Has("triggers.image.openshift.io/deployments"),
HasDaemonSetsEnabled: options.DisabledFeatures.Has("triggers.image.openshift.io/daemonsets"),
HasStatefulSetsEnabled: options.DisabledFeatures.Has("triggers.image.openshift.io/statefulsets"),
HasCronJobsEnabled: options.DisabledFeatures.Has("triggers.image.openshift.io/cronjobs"),
}
ret.ImageImportControllerConfig = ImageImportControllerConfig{
MaxScheduledImageImportsPerMinute: options.ImagePolicyConfig.MaxScheduledImageImportsPerMinute,
ResyncPeriod: 10 * time.Minute,
Expand Down
91 changes: 36 additions & 55 deletions pkg/cmd/openshift-controller-manager/controller/image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,7 @@ import (
triggerdeploymentconfigs "github.com/openshift/origin/pkg/image/trigger/deploymentconfigs"
)

type ImageTriggerControllerConfig struct {
HasBuilderEnabled bool
HasDeploymentsEnabled bool
HasDaemonSetsEnabled bool
HasStatefulSetsEnabled bool
HasCronJobsEnabled bool
}

func (c *ImageTriggerControllerConfig) RunController(ctx ControllerContext) (bool, error) {
//streamInformer := ctx.ImageInformers.Image().InternalVersion().ImageStreams().Informer()
func RunImageTriggerController(ctx ControllerContext) (bool, error) {
informer := ctx.ImageInformers.Image().InternalVersion().ImageStreams()

buildClient, err := ctx.ClientBuilder.OpenshiftInternalBuildClient(bootstrappolicy.InfraImageTriggerControllerServiceAccountName)
Expand All @@ -63,51 +54,41 @@ func (c *ImageTriggerControllerConfig) RunController(ctx ControllerContext) (boo
Reactor: &triggerdeploymentconfigs.DeploymentConfigReactor{Client: appsClient.Apps()},
},
}
if !c.HasBuilderEnabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

@deads2k was this even logically correct before? if has not builder enabled, then add trigger for build configs sounds weird to me?

Copy link
Contributor

Choose a reason for hiding this comment

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

i guess not :-) and nobody noticed this since 3.6... shame on me and shame on @smarterclayton who manually merged it ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

nvmd. false alarm, this code is unfortunate but it works because we check if the feature is disabled when we initialize this struct (the negation should have been there to make this more clear...)

sources = append(sources, imagetriggercontroller.TriggerSource{
Resource: schema.GroupResource{Group: "build.openshift.io", Resource: "buildconfigs"},
Informer: ctx.BuildInformers.Build().InternalVersion().BuildConfigs().Informer(),
Store: ctx.BuildInformers.Build().InternalVersion().BuildConfigs().Informer().GetIndexer(),
TriggerFn: triggerbuildconfigs.NewBuildConfigTriggerIndexer,
Reactor: triggerbuildconfigs.NewBuildConfigReactor(bcInstantiator, kclient.Core().RESTClient()),
})
}
if !c.HasDeploymentsEnabled {
sources = append(sources, imagetriggercontroller.TriggerSource{
Resource: schema.GroupResource{Group: "extensions", Resource: "deployments"},
Informer: ctx.ExternalKubeInformers.Extensions().V1beta1().Deployments().Informer(),
Store: ctx.ExternalKubeInformers.Extensions().V1beta1().Deployments().Informer().GetIndexer(),
TriggerFn: triggerannotations.NewAnnotationTriggerIndexer,
Reactor: &triggerannotations.AnnotationReactor{Updater: updater},
})
}
if !c.HasDaemonSetsEnabled {
sources = append(sources, imagetriggercontroller.TriggerSource{
Resource: schema.GroupResource{Group: "extensions", Resource: "daemonsets"},
Informer: ctx.ExternalKubeInformers.Extensions().V1beta1().DaemonSets().Informer(),
Store: ctx.ExternalKubeInformers.Extensions().V1beta1().DaemonSets().Informer().GetIndexer(),
TriggerFn: triggerannotations.NewAnnotationTriggerIndexer,
Reactor: &triggerannotations.AnnotationReactor{Updater: updater},
})
}
if !c.HasStatefulSetsEnabled {
sources = append(sources, imagetriggercontroller.TriggerSource{
Resource: schema.GroupResource{Group: "apps", Resource: "statefulsets"},
Informer: ctx.ExternalKubeInformers.Apps().V1beta1().StatefulSets().Informer(),
Store: ctx.ExternalKubeInformers.Apps().V1beta1().StatefulSets().Informer().GetIndexer(),
TriggerFn: triggerannotations.NewAnnotationTriggerIndexer,
Reactor: &triggerannotations.AnnotationReactor{Updater: updater},
})
}
if !c.HasCronJobsEnabled {
sources = append(sources, imagetriggercontroller.TriggerSource{
Resource: schema.GroupResource{Group: "batch", Resource: "cronjobs"},
Informer: ctx.ExternalKubeInformers.Batch().V2alpha1().CronJobs().Informer(),
Store: ctx.ExternalKubeInformers.Batch().V2alpha1().CronJobs().Informer().GetIndexer(),
TriggerFn: triggerannotations.NewAnnotationTriggerIndexer,
Reactor: &triggerannotations.AnnotationReactor{Updater: updater},
})
}
sources = append(sources, imagetriggercontroller.TriggerSource{
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm slightly worried we're enabling this, iirc this was explicitly disabled due to some problems. @tnozicka or @mfojtik mind remember some details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm slightly worried we're enabling this, iirc this was explicitly disabled due to some problems. @tnozicka or @mfojtik mind remember some details.

It would have failed validation here, right? https://github.com/openshift/origin/pull/19070/files#diff-6b6e279f3c86e5edb0688c1ca449db56L241

Resource: schema.GroupResource{Group: "build.openshift.io", Resource: "buildconfigs"},
Informer: ctx.BuildInformers.Build().InternalVersion().BuildConfigs().Informer(),
Store: ctx.BuildInformers.Build().InternalVersion().BuildConfigs().Informer().GetIndexer(),
TriggerFn: triggerbuildconfigs.NewBuildConfigTriggerIndexer,
Reactor: triggerbuildconfigs.NewBuildConfigReactor(bcInstantiator, kclient.Core().RESTClient()),
})
sources = append(sources, imagetriggercontroller.TriggerSource{
Resource: schema.GroupResource{Group: "extensions", Resource: "deployments"},
Informer: ctx.ExternalKubeInformers.Extensions().V1beta1().Deployments().Informer(),
Store: ctx.ExternalKubeInformers.Extensions().V1beta1().Deployments().Informer().GetIndexer(),
TriggerFn: triggerannotations.NewAnnotationTriggerIndexer,
Reactor: &triggerannotations.AnnotationReactor{Updater: updater},
})
sources = append(sources, imagetriggercontroller.TriggerSource{
Resource: schema.GroupResource{Group: "extensions", Resource: "daemonsets"},
Informer: ctx.ExternalKubeInformers.Extensions().V1beta1().DaemonSets().Informer(),
Store: ctx.ExternalKubeInformers.Extensions().V1beta1().DaemonSets().Informer().GetIndexer(),
TriggerFn: triggerannotations.NewAnnotationTriggerIndexer,
Reactor: &triggerannotations.AnnotationReactor{Updater: updater},
})
sources = append(sources, imagetriggercontroller.TriggerSource{
Resource: schema.GroupResource{Group: "apps", Resource: "statefulsets"},
Informer: ctx.ExternalKubeInformers.Apps().V1beta1().StatefulSets().Informer(),
Store: ctx.ExternalKubeInformers.Apps().V1beta1().StatefulSets().Informer().GetIndexer(),
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're at it can you update all these to use the Apps().V1() for all of the above 3 (deployments, daemonsets and statefulsets).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While you're at it can you update all these to use the Apps().V1() for all of the above 3 (deployments, daemonsets and statefulsets).

Doesn't that ripple through the types in the controller too? I'd have to update all the trigger funcs, rigth?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I'll create a followup.

TriggerFn: triggerannotations.NewAnnotationTriggerIndexer,
Reactor: &triggerannotations.AnnotationReactor{Updater: updater},
})
sources = append(sources, imagetriggercontroller.TriggerSource{
Resource: schema.GroupResource{Group: "batch", Resource: "cronjobs"},
Informer: ctx.ExternalKubeInformers.Batch().V2alpha1().CronJobs().Informer(),
Store: ctx.ExternalKubeInformers.Batch().V2alpha1().CronJobs().Informer().GetIndexer(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, but Batch().V1beta1().

TriggerFn: triggerannotations.NewAnnotationTriggerIndexer,
Reactor: &triggerannotations.AnnotationReactor{Updater: updater},
})

go imagetriggercontroller.NewTriggerController(
broadcaster,
Expand Down
21 changes: 0 additions & 21 deletions pkg/cmd/openshift-controller-manager/controller_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,13 @@ import (
"fmt"
"net/http"
"os"
"strings"
"time"

"github.com/golang/glog"

"k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
kutilerrors "k8s.io/apimachinery/pkg/util/errors"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/wait"
clientgoclientset "k8s.io/client-go/kubernetes"
kclientsetexternal "k8s.io/client-go/kubernetes"
Expand Down Expand Up @@ -63,9 +61,6 @@ func runOpenShiftControllerManager(masterConfig *configapi.MasterConfig, runServ
// you can't double run healthz, so only do this next bit if we aren't starting the API
if runServer {
glog.Infof("Starting controllers on %s (%s)", masterConfig.ServingInfo.BindAddress, version.Get().String())
if len(masterConfig.DisabledFeatures) > 0 {
glog.V(4).Infof("Disabled features: %s", strings.Join(masterConfig.DisabledFeatures, ", "))
}

if err := origincontrollers.RunControllerServer(masterConfig.ServingInfo, clientGoKubeExternal); err != nil {
return err
Expand Down Expand Up @@ -280,14 +275,7 @@ func startControllers(options configapi.MasterConfig, allocationController origi
return err
}

excludedControllers := getExcludedControllers(options)

for controllerName, initFn := range openshiftControllerInitializers {
// TODO remove this. Only call one to start to prove the principle
if excludedControllers.Has(controllerName) {
glog.Warningf("%q is skipped", controllerName)
continue
}
if !controllerContext.IsControllerEnabled(controllerName) {
glog.Warningf("%q is disabled", controllerName)
continue
Expand All @@ -310,12 +298,3 @@ func startControllers(options configapi.MasterConfig, allocationController origi

return nil
}

func getExcludedControllers(options configapi.MasterConfig) sets.String {
excludedControllers := sets.NewString()
if !configapi.IsBuildEnabled(&options) {
excludedControllers.Insert("openshift.io/build")
excludedControllers.Insert("openshift.io/build-config-change")
}
return excludedControllers
}
4 changes: 0 additions & 4 deletions pkg/cmd/openshift-kube-apiserver/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package openshift_kube_apiserver

import (
"fmt"
"strings"

"github.com/golang/glog"

Expand Down Expand Up @@ -67,9 +66,6 @@ func RunOpenShiftKubeAPIServerServer(masterConfig *configapi.MasterConfig) error

glog.Infof("Starting master on %s (%s)", masterConfig.ServingInfo.BindAddress, version.Get().String())
glog.Infof("Public master address is %s", masterConfig.MasterPublicURL)
if len(masterConfig.DisabledFeatures) > 0 {
glog.V(4).Infof("Disabled features: %s", strings.Join(masterConfig.DisabledFeatures, ", "))
}
imageTemplate := variable.NewDefaultImageTemplate()
imageTemplate.Format = masterConfig.ImageConfig.Format
imageTemplate.Latest = masterConfig.ImageConfig.Latest
Expand Down
74 changes: 0 additions & 74 deletions pkg/cmd/server/apis/config/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,80 +23,6 @@ import (
cmdutil "github.com/openshift/origin/pkg/cmd/util"
)

var (
// Maps lower-cased feature flag names and aliases to their canonical names.
knownOpenShiftFeatureSet map[string]string
)

func init() {
knownOpenShiftFeatureSet = make(map[string]string, len(KnownOpenShiftFeatures))
for _, feature := range KnownOpenShiftFeatures {
knownOpenShiftFeatureSet[strings.ToLower(feature)] = feature
}
for alias, feature := range FeatureAliases {
knownOpenShiftFeatureSet[strings.ToLower(alias)] = feature
}
}

// NormalizeOpenShiftFeature returns canonical name for given OpenShift feature
// flag or an alias if known. Otherwise lower-cased name is returned.
func NormalizeOpenShiftFeature(name string) (string, bool) {
name = strings.ToLower(name)
if feature, ok := knownOpenShiftFeatureSet[name]; ok {
return feature, true
}
return name, false
}

// Add extends feature list with given valid items. They are appended
// unless already present.
func (fl *FeatureList) Add(items ...string) error {
unknown := []string{}
toAppend := make([]string, 0, len(items))
for _, item := range items {
feature, known := NormalizeOpenShiftFeature(item)
if !known {
unknown = append(unknown, item)
continue
}
if fl.Has(feature) {
continue
}
toAppend = append(toAppend, feature)
}
if len(unknown) > 0 {
return fmt.Errorf("unknown features: %s", strings.Join(unknown, ", "))
}
*fl = append(*fl, toAppend...)
return nil
}

// Delete removes given items from feature list while keeping its original
// order.
func (fl *FeatureList) Delete(items ...string) {
toDelete := FeatureList(items)
newList := []string{}
for _, item := range *fl {
if !toDelete.Has(item) {
newList = append(newList, item)
}
}
*fl = newList
}

// Has returns true if given feature exists in feature list. The check is
// case-insensitive.
func (fl FeatureList) Has(feature string) bool {
normalized, _ := NormalizeOpenShiftFeature(feature)
for _, item := range fl {
itemNormalized, _ := NormalizeOpenShiftFeature(item)
if normalized == itemNormalized {
return true
}
}
return false
}

// ParseNamespaceAndName returns back the namespace and name (empty if something goes wrong), for a given string.
// This is useful when pointing to a particular resource inside of our config.
func ParseNamespaceAndName(in string) (string, string, error) {
Expand Down
18 changes: 0 additions & 18 deletions pkg/cmd/server/apis/config/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,6 @@ import (

// A new entry shall be added to FeatureAliases for every change to following values.
const (
FeatureBuilder = `Builder`
FeatureS2I = `S2IBuilder`
FeatureWebConsole = `WebConsole`

AllVersions = "*"
)

Expand Down Expand Up @@ -135,15 +131,6 @@ var (
KnownKubeAPIGroups = sets.StringKeySet(KubeAPIGroupsToAllowedVersions)
KnownOriginAPIGroups = sets.StringKeySet(OriginAPIGroupsToAllowedVersions)

// FeatureAliases maps deprecated names of feature flag to their canonical
// names. Aliases must be lower-cased for O(1) lookup.
FeatureAliases = map[string]string{
"s2i builder": FeatureS2I,
"web console": FeatureWebConsole,
}
KnownOpenShiftFeatures = []string{FeatureBuilder, FeatureS2I, FeatureWebConsole}
AtomicDisabledFeatures = []string{FeatureBuilder, FeatureS2I, FeatureWebConsole}

// List public registries that we are allowing to import images from by default.
// By default all registries have set to be "secure", iow. the port for them is
// defaulted to "443".
Expand Down Expand Up @@ -330,8 +317,6 @@ const (
ControllersAll = "*"
)

type FeatureList []string

// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

type MasterConfig struct {
Expand Down Expand Up @@ -379,9 +364,6 @@ type MasterConfig struct {

ControllerConfig ControllerConfig

// Allow to disable OpenShift components
DisabledFeatures FeatureList

// EtcdStorageConfig contains information about how API resources are
// stored in Etcd. These values are only relevant when etcd is the
// backing store for the cluster.
Expand Down
Loading