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

Jump to bottom

DaemonSet: Respect ControllerRef #42173

Merged
merged 10 commits into from
Mar 8, 2017
Merged

DaemonSet: Respect ControllerRef #42173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8f3a56f
DaemonSet: Add ControllerRef on all created Pods.
enisoc Feb 25, 2017
2217363
DaemonSet: Set DefaultGarbageCollectionPolicy to OrphanDependents.
enisoc Feb 25, 2017
421e0bb
DaemonSet: Use ControllerRefManager to adopt/orphan.
enisoc Feb 26, 2017
1099811
DaemonSet: Use ControllerRef to route watch events.
enisoc Feb 26, 2017
c288f52
DaemonSet: Update Lister documentation for ControllerRef.
enisoc Feb 26, 2017
ab5a82d
DaemonSet: Don't log Pod events unless some DaemonSet cares.
enisoc Mar 2, 2017
97c363a
DaemonSet: Always set BlockOwnerDeletion in ControllerRef.
enisoc Mar 2, 2017
182753f
DaemonSet: Check that ControllerRef UID matches.
enisoc Mar 6, 2017
e2deb17
DaemonSet: Mark daemonset-lookup-cache-size flag as deprecated.
enisoc Mar 7, 2017
fac372d
DaemonSet: Relist Pods before each phase of sync.
enisoc Mar 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion cmd/kube-controller-manager/app/extensions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ func startDaemonSetController(ctx ControllerContext) (bool, error) {
ctx.InformerFactory.Core().V1().Pods(),
ctx.InformerFactory.Core().V1().Nodes(),
ctx.ClientBuilder.ClientOrDie("daemon-set-controller"),
int(ctx.Options.LookupCacheSizeForDaemonSet),
).Run(int(ctx.Options.ConcurrentDaemonSetSyncs), ctx.Stop)
return true, nil
}
Expand Down
4 changes: 3 additions & 1 deletion cmd/kube-controller-manager/app/options/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,9 @@ func (s *CMServer) AddFlags(fs *pflag.FlagSet, allControllers []string, disabled
fs.Int32Var(&s.ConcurrentSATokenSyncs, "concurrent-serviceaccount-token-syncs", s.ConcurrentSATokenSyncs, "The number of service account token objects that are allowed to sync concurrently. Larger number = more responsive token generation, but more CPU (and network) load")
fs.Int32Var(&s.LookupCacheSizeForRC, "replication-controller-lookup-cache-size", s.LookupCacheSizeForRC, "This flag is deprecated and will be removed in future releases. ReplicationController no longer requires a lookup cache.")
fs.Int32Var(&s.LookupCacheSizeForRS, "replicaset-lookup-cache-size", s.LookupCacheSizeForRS, "This flag is deprecated and will be removed in future releases. ReplicaSet no longer requires a lookup cache.")
fs.Int32Var(&s.LookupCacheSizeForDaemonSet, "daemonset-lookup-cache-size", s.LookupCacheSizeForDaemonSet, "The the size of lookup cache for daemonsets. Larger number = more responsive daemonsets, but more MEM load.")
// TODO: Remove the following flag 6 months after v1.6.0 is released.
Copy link
Member

@janetkuo janetkuo Mar 7, 2017 •
edited

Choose a reason for hiding this comment

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

We should call fs.MarkDeprecated, and put a TODO for when to remove this flag
(do this in other ControllerRef PRs too)

Edit: see https://kubernetes.io/docs/deprecation-policy/#deprecating-a-flag-or-cli, we can remove this flag 6 months after deprecating it

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks I fixed it here and added it to my list of fixes for RC/RS that I will make in a separate PR.

Copy link
Member

@janetkuo janetkuo Mar 8, 2017 •
edited

Choose a reason for hiding this comment

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

Copying from my comment in https://github.com/kubernetes/kubernetes/pull/42659/files#r104817545:

MarkDeprecated prints deprecation warnings for you, but we still need these flags.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

fs.Int32Var(&s.LookupCacheSizeForDaemonSet, "daemonset-lookup-cache-size", s.LookupCacheSizeForDaemonSet, "This flag is deprecated and will be removed in future releases. DaemonSet no longer requires a lookup cache.")
fs.MarkDeprecated("daemonset-lookup-cache-size", "This flag is deprecated and will be removed in future releases. DaemonSet no longer requires a lookup cache.")
fs.DurationVar(&s.ServiceSyncPeriod.Duration, "service-sync-period", s.ServiceSyncPeriod.Duration, "The period for syncing services with their external load balancers")
fs.DurationVar(&s.NodeSyncPeriod.Duration, "node-sync-period", 0, ""+
"This flag is deprecated and will be removed in future releases. See node-monitor-period for Node health checking or "+
Expand Down
1 change: 1 addition & 0 deletions pkg/apis/componentconfig/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,7 @@ type KubeControllerManagerConfiguration struct {
LookupCacheSizeForRS int32
// lookupCacheSizeForDaemonSet is the size of lookup cache for daemonsets.
// Larger number = more responsive daemonset, but more MEM load.
// DEPRECATED: This is no longer used.
LookupCacheSizeForDaemonSet int32
// serviceSyncPeriod is the period for syncing services with their external
// load balancers.
Expand Down
5 changes: 3 additions & 2 deletions pkg/client/listers/extensions/internalversion/daemonset_expansion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,9 @@ type DaemonSetListerExpansion interface {
// DaemonSetNamespaeLister.
type DaemonSetNamespaceListerExpansion interface{}

// GetPodDaemonSets returns a list of daemon sets managing a pod.
// Returns an error if and only if no matching daemon sets are found.
// GetPodDaemonSets returns a list of DaemonSets that potentially match a pod.
// Only the one specified in the Pod's ControllerRef will actually manage it.
// Returns an error only if no matching DaemonSets are found.
func (s *daemonSetLister) GetPodDaemonSets(pod *api.Pod) ([]*extensions.DaemonSet, error) {
var selector labels.Selector
var daemonSet *extensions.DaemonSet
Expand Down
5 changes: 3 additions & 2 deletions pkg/client/listers/extensions/v1beta1/daemonset_expansion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,9 @@ type DaemonSetListerExpansion interface {
// DaemonSetNamespaeLister.
type DaemonSetNamespaceListerExpansion interface{}

// GetPodDaemonSets returns a list of daemon sets managing a pod.
// Returns an error if and only if no matching daemon sets are found.
// GetPodDaemonSets returns a list of DaemonSets that potentially match a pod.
// Only the one specified in the Pod's ControllerRef will actually manage it.
// Returns an error only if no matching DaemonSets are found.
func (s *daemonSetLister) GetPodDaemonSets(pod *v1.Pod) ([]*v1beta1.DaemonSet, error) {
var selector labels.Selector
var daemonSet *v1beta1.DaemonSet
Expand Down
34 changes: 23 additions & 11 deletions pkg/controller/controller_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,8 +400,9 @@ func (r RealRSControl) PatchReplicaSet(namespace, name string, data []byte) erro
type PodControlInterface interface {
// CreatePods creates new pods according to the spec.
CreatePods(namespace string, template *v1.PodTemplateSpec, object runtime.Object) error
// CreatePodsOnNode creates a new pod according to the spec on the specified node.
CreatePodsOnNode(nodeName, namespace string, template *v1.PodTemplateSpec, object runtime.Object) error
// CreatePodsOnNode creates a new pod according to the spec on the specified node,
// and sets the ControllerRef.
CreatePodsOnNode(nodeName, namespace string, template *v1.PodTemplateSpec, object runtime.Object, controllerRef *metav1.OwnerReference) error
// CreatePodsWithControllerRef creates new pods according to the spec, and sets object as the pod's controller.
CreatePodsWithControllerRef(namespace string, template *v1.PodTemplateSpec, object runtime.Object, controllerRef *metav1.OwnerReference) error
// DeletePod deletes the pod identified by podID.
Expand Down Expand Up @@ -466,11 +467,7 @@ func getPodsPrefix(controllerName string) string {
return prefix
}

func (r RealPodControl) CreatePods(namespace string, template *v1.PodTemplateSpec, object runtime.Object) error {
return r.createPods("", namespace, template, object, nil)
}

func (r RealPodControl) CreatePodsWithControllerRef(namespace string, template *v1.PodTemplateSpec, controllerObject runtime.Object, controllerRef *metav1.OwnerReference) error {
func validateControllerRef(controllerRef *metav1.OwnerReference) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this validation running in the api server or only in controllers?

Copy link
Member

Choose a reason for hiding this comment

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

Only in controllers.

Copy link
Member

Choose a reason for hiding this comment

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

This validation is only required for controllers. API server doesn't know if a request is sent by a controller thus cannot do the validation.

If a TPR creates dependents, then its custom controller could do a similar check.

if controllerRef == nil {
return fmt.Errorf("controllerRef is nil")
}
Expand All @@ -481,16 +478,30 @@ func (r RealPodControl) CreatePodsWithControllerRef(namespace string, template *
return fmt.Errorf("controllerRef has empty Kind")
}
if controllerRef.Controller == nil || *controllerRef.Controller != true {
Copy link
Member

Choose a reason for hiding this comment

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

Could you also verify that controllerRef.BlockOwnerDeletion is true, and set it to true in all controllee's creation/adoption? See the comments here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will do this in a separate PR since the change will apply to all controllers.

Copy link
Member Author

Choose a reason for hiding this comment

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

After looking more closely, it makes more sense to fix it here. I just needed to move the BlockOwnerDeletion check up into validateControllerRef().

Copy link
Member

Choose a reason for hiding this comment

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

Should we also validate that controllerRef's Name and UID isn't empty (i.e. len > 0)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make a note to address that in a future PR. It will affect multiple controllers and may require updates to their unit tests, since not all tests bother to fill those in. In this PR, I am just moving this validation code to a common function so I can use it in two places without duplicating.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good

return fmt.Errorf("controllerRef.Controller is not set")
return fmt.Errorf("controllerRef.Controller is not set to true")
}
if controllerRef.BlockOwnerDeletion == nil || *controllerRef.BlockOwnerDeletion != true {
return fmt.Errorf("controllerRef.BlockOwnerDeletion is not set")
}
return nil
}

func (r RealPodControl) CreatePods(namespace string, template *v1.PodTemplateSpec, object runtime.Object) error {
return r.createPods("", namespace, template, object, nil)
}

func (r RealPodControl) CreatePodsWithControllerRef(namespace string, template *v1.PodTemplateSpec, controllerObject runtime.Object, controllerRef *metav1.OwnerReference) error {
if err := validateControllerRef(controllerRef); err != nil {
return err
}
return r.createPods("", namespace, template, controllerObject, controllerRef)
}

func (r RealPodControl) CreatePodsOnNode(nodeName, namespace string, template *v1.PodTemplateSpec, object runtime.Object) error {
return r.createPods(nodeName, namespace, template, object, nil)
func (r RealPodControl) CreatePodsOnNode(nodeName, namespace string, template *v1.PodTemplateSpec, object runtime.Object, controllerRef *metav1.OwnerReference) error {
if err := validateControllerRef(controllerRef); err != nil {
return err
}
return r.createPods(nodeName, namespace, template, object, controllerRef)
}

func (r RealPodControl) PatchPod(namespace, name string, data []byte) error {
Expand Down Expand Up @@ -613,10 +624,11 @@ func (f *FakePodControl) CreatePodsWithControllerRef(namespace string, spec *v1.
return nil
}

func (f *FakePodControl) CreatePodsOnNode(nodeName, namespace string, template *v1.PodTemplateSpec, object runtime.Object) error {
func (f *FakePodControl) CreatePodsOnNode(nodeName, namespace string, template *v1.PodTemplateSpec, object runtime.Object, controllerRef *metav1.OwnerReference) error {
f.Lock()
defer f.Unlock()
f.Templates = append(f.Templates, *template)
f.ControllerRefs = append(f.ControllerRefs, *controllerRef)
if f.Err != nil {
return f.Err
}
Expand Down
2 changes: 2 additions & 0 deletions pkg/controller/daemon/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,13 @@ go_test(
"//vendor:k8s.io/apimachinery/pkg/apis/meta/v1",
"//vendor:k8s.io/apimachinery/pkg/runtime",
"//vendor:k8s.io/apimachinery/pkg/util/intstr",
"//vendor:k8s.io/apimachinery/pkg/util/uuid",
"//vendor:k8s.io/apiserver/pkg/storage/names",
"//vendor:k8s.io/apiserver/pkg/util/feature",
"//vendor:k8s.io/client-go/testing",
"//vendor:k8s.io/client-go/tools/cache",
"//vendor:k8s.io/client-go/tools/record",
"//vendor:k8s.io/client-go/util/workqueue",
],
)

Expand Down