Repository: istio/istio
File: cni/pkg/nodeagent/server.go (Go)

Code before:
// ShouldStopCleanup of istio-cni config and binary when upgrading or on node reboot
func (s *Server) ShouldStopCleanup(selfName, selfNamespace string) bool {
	dsName := fmt.Sprintf("%s-node", selfName)
	shouldStopCleanup := true
	err := backoff.Retry(
		func() error {
			cniDS, err := s.kubeClient.Kube().AppsV1().DaemonSets(selfNamespace).Get(context.Background(), dsName, metav1.GetOptions{})

			log.Debugf("Daemonset %s has deletion timestamp?: %+v", dsName, cniDS.DeletionTimestamp)
			if err == nil && cniDS != nil && cniDS.DeletionTimestamp == nil {
				log.Infof("terminating, but parent DS %s is still present, this is an upgrade or a node reboot, leaving plugin in place", dsName)
				shouldStopCleanup = true
				return nil
			}
			// TODO(jaellio): Not certain "not found" error can be interpreted as not a node reboot
			if errors.IsNotFound(err) || (cniDS != nil && cniDS.DeletionTimestamp != nil) {
				// If the DS is gone, or marked for deletion, this is not an upgrade.
				// We can safely shut down the plugin.
				log.Infof("parent DS %s is not found or marked for deletion, this is not an upgrade, shutting down normally", dsName)
				shouldStopCleanup = false
				return nil
			}
			log.Infof("failed to get parent DS %s, retrying: %v", dsName, err)
			return err
		},
		backoff.WithMaxRetries(backoff.NewConstantBackOff(tokenWaitBackoff), 5))
	if err != nil {
		log.Infof("failed to get parent DS %s, returning %s: %v", dsName, shouldStopCleanup, err)
	}
	return shouldStopCleanup
}

// buildKubeClient creates the kube client
func buildKubeClient(kubeConfig string) (kube.Client, error) {
	// Used by validation
	kubeRestConfig, err := kube.DefaultRestConfig(kubeConfig, "", func(config *rest.Config) {
		config.QPS = 80
		config.Burst = 160
	})
	if err != nil {
		return nil, fmt.Errorf("failed creating kube config: %v", err)
	}

	client, err := kube.NewClient(kube.NewClientConfigForRestConfig(kubeRestConfig), "")
	if err != nil { 
		return nil, fmt.Errorf("failed creating kube client: %v", err)
	}

	return client, nil
}


Review comment (bug):
Are these failures predictable and deterministic, or more likely transient? If failures repeat, could constant-interval retries risk causing a retry storm? Would exponential backoff (possibly with jitter) be a safer approach here?

Code after:
// ShouldStopCleanup of istio-cni config and binary when upgrading or on node reboot
func (s *Server) ShouldStopCleanup(selfName, selfNamespace string, istioOwnedCNIConfig bool) bool {
	dsName := fmt.Sprintf("%s-node", selfName)
	shouldStopCleanup := false
	var numRetries uint64
	// use different defaults when using an istio owned CNI config file
	if istioOwnedCNIConfig {
		shouldStopCleanup = true
		numRetries = 2
	}
	err := backoff.Retry(
		func() error {
			cniDS, err := s.kubeClient.Kube().AppsV1().DaemonSets(selfNamespace).Get(context.Background(), dsName, metav1.GetOptions{})

			if err == nil && cniDS != nil && cniDS.DeletionTimestamp == nil {
				log.Infof("terminating, but parent DaemonSet %s is still present, this is an upgrade or a node reboot, leaving plugin in place", dsName)
				shouldStopCleanup = true
				return nil
			}
			if errors.IsNotFound(err) || (cniDS != nil && cniDS.DeletionTimestamp != nil) {
				// If the DS is gone, or marked for deletion, this is not an upgrade.
				// We can safely shut down the plugin.
				log.Infof("parent DaemonSet %s is not found or marked for deletion, this is not an upgrade, shutting down normally", dsName)
				shouldStopCleanup = false
				return nil
			}
			if errors.IsUnauthorized(err) {
				log.Infof("permission to get parent DaemonSet %s has been revoked manually or due to uninstall, this is not an upgrade, "+
					"shutting down normally", dsName)
				shouldStopCleanup = false
				return nil
			}
			log.Infof("failed to get parent DS %s, retrying: %v", dsName, err)
			return err
		},
		// Limiting retries to 3 so other shutdown tasks can complete before the graceful shutdown period ends
		backoff.WithMaxRetries(backoff.NewConstantBackOff(tokenWaitBackoff), numRetries))
	if err != nil {
		log.Infof("failed to get parent DaemonSet %s, returning %t: %v", dsName, shouldStopCleanup, err)
	}
	return shouldStopCleanup
}

// buildKubeClient creates the kube client
func buildKubeClient(kubeConfig string) (kube.Client, error) {
	// Used by validation
	kubeRestConfig, err := kube.DefaultRestConfig(kubeConfig, "", func(config *rest.Config) {
		config.QPS = 80
		config.Burst = 160
	})
	if err != nil {