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.

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

Allow configuration of import pod restart policy #3619

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 18 additions & 0 deletions api/openapi-spec/swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -3987,6 +3987,15 @@
"description": "Preallocation controls whether storage for DataVolumes should be allocated in advance.",
"type": "boolean"
},
"restartPolicy": {
"description": "RestartPolicy describes how the container should be restarted.\n\nPossible enum values:\n - `\"Always\"`\n - `\"Never\"`\n - `\"OnFailure\"`",
"type": "string",
"enum": [
"Always",
"Never",
"OnFailure"
]
},
"scratchSpaceStorageClass": {
"description": "Override the storage class to used for scratch space during transfer operations. The scratch space storage class is determined in the following order: 1. value of scratchSpaceStorageClass, if that doesn't exist, use the default storage class, if there is no default storage class, use the storage class of the DataVolume, if no storage class specified, use no storage class for scratch space",
"type": "string"
Expand Down Expand Up @@ -4029,6 +4038,15 @@
"description": "Preallocation controls whether storage for DataVolumes should be allocated in advance.",
"type": "boolean"
},
"restartPolicy": {
"description": "RestartPolicy describes how the container should be restarted.\n\nPossible enum values:\n - `\"Always\"`\n - `\"Never\"`\n - `\"OnFailure\"`",
"type": "string",
"enum": [
"Always",
"Never",
"OnFailure"
]
},
"scratchSpaceStorageClass": {
"description": "The calculated storage class to be used for scratch space",
"type": "string"
Expand Down
16 changes: 16 additions & 0 deletions pkg/apis/core/v1beta1/openapi_generated.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions pkg/common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,8 @@ const (
ImporterCertDir = "/certs"
// DefaultPullPolicy imports k8s "IfNotPresent" string for the import_controller_gingko_test and the cdi-controller executable
DefaultPullPolicy = string(v1.PullIfNotPresent)
// DefaultRestartPolicy imports k8s "OnFailure" string for the import-controller_test and cdi-controller executable
DefaultRestartPolicy = string(v1.RestartPolicyOnFailure)
// ImportProxyConfigMapName provides the key for getting the name of the ConfigMap in the cdi namespace containing a CA certificate bundle
ImportProxyConfigMapName = "trusted-ca-proxy-bundle-cm"
// ImportProxyConfigMapKey provides the key name of the ConfigMap in the cdi namespace containing a CA certificate bundle
Expand Down
10 changes: 10 additions & 0 deletions pkg/controller/common/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,16 @@ func GetDefaultPodResourceRequirements(client client.Client) (*corev1.ResourceRe
return cdiconfig.Status.DefaultPodResourceRequirements, nil
}

// GetImportPodRestartPolicy gets the current import pod restart policy from cdi config
func GetImportPodRestartPolicy(client client.Client) (corev1.RestartPolicy, error) {
cdiconfig := &cdiv1.CDIConfig{}
if err := client.Get(context.TODO(), types.NamespacedName{Name: common.ConfigName}, cdiconfig); err != nil {
klog.Errorf("Unable to find CDI configuration, %v\n", err)
return "", err
}
return cdiconfig.Status.ImportPodRestartPolicy, nil
}

// GetImagePullSecrets gets the imagePullSecrets needed to pull images from the cdi config
func GetImagePullSecrets(client client.Client) ([]corev1.LocalObjectReference, error) {
cdiconfig := &cdiv1.CDIConfig{}
Expand Down
15 changes: 14 additions & 1 deletion pkg/controller/import-controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ type importerPodArgs struct {
vddkImageName *string
vddkExtraArgs *string
priorityClassName string
restartPolicy string
}

// NewImportController creates a new instance of the import controller.
Expand Down Expand Up @@ -867,6 +868,18 @@ func createImporterPod(ctx context.Context, log logr.Logger, client client.Clien
return nil, err
}

var restartPolicy corev1.RestartPolicy
restartPolicy, err = cc.GetImportPodRestartPolicy(client)
if err != nil {
return nil, err
}
if args.restartPolicy == "" && restartPolicy == "" {
args.restartPolicy = common.DefaultRestartPolicy
}
if restartPolicy != "" {
args.restartPolicy = string(restartPolicy)
}

if isRegistryNodeImport(args) {
args.importImage, err = getRegistryImportImage(args.pvc)
if err != nil {
Expand Down Expand Up @@ -923,7 +936,7 @@ func makeImporterPodSpec(args *importerPodArgs) *corev1.Pod {
Containers: makeImporterContainerSpec(args),
InitContainers: makeImporterInitContainersSpec(args),
Volumes: makeImporterVolumeSpec(args),
RestartPolicy: corev1.RestartPolicyOnFailure,
RestartPolicy: v1.RestartPolicy(args.restartPolicy),
NodeSelector: args.workloadNodePlacement.NodeSelector,
Tolerations: args.workloadNodePlacement.Tolerations,
Affinity: args.workloadNodePlacement.Affinity,
Expand Down
42 changes: 39 additions & 3 deletions pkg/controller/import-controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,10 @@ import (
)

const (
testEndPoint = "http://test.somewhere.tt.blah"
testImage = "test/image"
testPullPolicy = "Always"
testEndPoint = "http://test.somewhere.tt.blah"
testImage = "test/image"
testPullPolicy = "Always"
testRestartPolicy = corev1.RestartPolicyOnFailure
)

var (
Expand Down Expand Up @@ -936,6 +937,41 @@ var _ = Describe("Create Importer Pod", func() {
Entry("with long PVC and checkpoint names", strings.Repeat("test-pvc-", 20), strings.Repeat("repeating-checkpoint-id-", 10)),
)

DescribeTable("should use the correct restart policy", func(restartPolicy string, expectedPolicy corev1.RestartPolicy) {
pvc := cc.CreatePvc(pvcName, "default", map[string]string{cc.AnnEndpoint: testEndPoint, cc.AnnImportPod: "podName"}, nil)
reconciler := createImportReconciler(pvc)
podEnvVar := &importPodEnvVar{
ep: "",
httpProxy: "",
httpsProxy: "",
secretName: "",
source: "",
contentType: "",
imageSize: "1G",
certConfigMap: "",
diskID: "",
filesystemOverhead: "0.055",
insecureTLS: false,
}
podArgs := &importerPodArgs{
image: testImage,
verbose: "5",
pullPolicy: testPullPolicy,
restartPolicy: restartPolicy,
podEnvVar: podEnvVar,
pvc: pvc,
scratchPvcName: &scratchPvcName,
priorityClassName: pvc.Annotations[cc.AnnPriorityClassName],
}
pod, err := createImporterPod(context.TODO(), reconciler.log, reconciler.client, podArgs, map[string]string{})
Expect(err).ToNot(HaveOccurred())
Expect(pod.Spec.RestartPolicy).To(BeEquivalentTo(expectedPolicy))
},
Entry("with an on failure restart policy", string(corev1.RestartPolicyOnFailure), corev1.RestartPolicyOnFailure),
Entry("with a never restart policy", string(corev1.RestartPolicyNever), corev1.RestartPolicyNever),
Entry("without a specified restart policy", "", corev1.RestartPolicy(common.DefaultRestartPolicy)),
)

It("should mount extra VDDK arguments ConfigMap when annotation is set", func() {
pvcName := "testPvc1"
podName := "testpod"
Expand Down
14 changes: 14 additions & 0 deletions pkg/operator/resources/crds_generated.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -1001,6 +1001,9 @@ type CDIConfigSpec struct {
ScratchSpaceStorageClass *string `json:"scratchSpaceStorageClass,omitempty"`
// ResourceRequirements describes the compute resource requirements.
PodResourceRequirements *corev1.ResourceRequirements `json:"podResourceRequirements,omitempty"`
// RestartPolicy describes how the container should be restarted.
// +kubebuilder:validation::Enum=Always;OnFailure;Never
ImportPodRestartPolicy corev1.RestartPolicy `json:"restartPolicy,omitempty"`
// FeatureGates are a list of specific enabled feature gates
FeatureGates []string `json:"featureGates,omitempty"`
// FilesystemOverhead describes the space reserved for overhead when using Filesystem volumes. A value is between 0 and 1, if not defined it is 0.055 (5.5% overhead)
Expand Down Expand Up @@ -1031,6 +1034,9 @@ type CDIConfigStatus struct {
// ImportProxy contains importer pod proxy configuration.
// +optional
ImportProxy *ImportProxy `json:"importProxy,omitempty"`
// RestartPolicy describes how the container should be restarted.
// +kubebuilder:validation::Enum=Always;OnFailure;Never
ImportPodRestartPolicy corev1.RestartPolicy `json:"restartPolicy,omitempty"`
// The calculated storage class to be used for scratch space
ScratchSpaceStorageClass string `json:"scratchSpaceStorageClass,omitempty"`
// ResourceRequirements describes the compute resource requirements.
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.