-
Notifications
You must be signed in to change notification settings - Fork 386
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
Optimize qos initializer. #878
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 |
---|---|---|
|
@@ -4,12 +4,16 @@ import ( | |
"context" | ||
"fmt" | ||
|
||
corev1 "k8s.io/api/core/v1" | ||
"github.com/gocrane/crane/pkg/ensurance/util" | ||
payall4u marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"github.com/pkg/errors" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/labels" | ||
|
||
corev1 "k8s.io/api/core/v1" | ||
"k8s.io/apimachinery/pkg/runtime" | ||
"k8s.io/klog/v2" | ||
|
||
"github.com/gocrane/api/ensurance/v1alpha1" | ||
"github.com/gocrane/crane/pkg/ensurance/config" | ||
) | ||
|
||
|
@@ -18,7 +22,15 @@ var ( | |
) | ||
|
||
type MutatingAdmission struct { | ||
Config *config.QOSConfig | ||
Config *config.QOSConfig | ||
listPodQOS func() ([]*v1alpha1.PodQOS, error) | ||
} | ||
|
||
func NewMutatingAdmission(config *config.QOSConfig, listPodQOS func() ([]*v1alpha1.PodQOS, error)) *MutatingAdmission { | ||
return &MutatingAdmission{ | ||
Config: config, | ||
listPodQOS: listPodQOS, | ||
} | ||
} | ||
|
||
// Default implements webhook.Defaulter so a webhook will be registered for the type | ||
|
@@ -28,7 +40,7 @@ func (m *MutatingAdmission) Default(ctx context.Context, obj runtime.Object) err | |
return fmt.Errorf("expected a Pod but got a %T", obj) | ||
} | ||
|
||
klog.Infof("Into Pod injection %s/%s", pod.Namespace, pod.Name) | ||
klog.Infof("mutating started for pod %s/%s", pod.Namespace, pod.Name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A code standard, all logs start with Capital letters, and please check if a log level should be added and disabled the msg by default. same suggestion to all newly added logs |
||
|
||
if _, exist := SystemNamespaces[pod.Namespace]; exist { | ||
return nil | ||
|
@@ -47,17 +59,54 @@ func (m *MutatingAdmission) Default(ctx context.Context, obj runtime.Object) err | |
return err | ||
} | ||
|
||
if ls.Matches(labels.Set(pod.Labels)) { | ||
if m.Config.QOSInitializer.InitContainerTemplate != nil { | ||
pod.Spec.InitContainers = append(pod.Spec.InitContainers, *m.Config.QOSInitializer.InitContainerTemplate) | ||
if !ls.Matches(labels.Set(pod.Labels)) { | ||
klog.Infof("injection skipped: webhook is not interested in the pod") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. injection->Injection, and this is a debug log, I suggest to add a log level and do not enable by default, and if we have label config for the webhook, this line of code should not be called? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In fact, I don't like this logic. But in order to be compatible with the previous design, I still leave the matching here. |
||
return nil | ||
} | ||
|
||
qosSlice, err := m.listPodQOS() | ||
if err != nil { | ||
return errors.WithMessage(err, "list PodQOS failed") | ||
} | ||
|
||
/**************************************************************** | ||
* Check whether the pod has a low CPUPriority (CPUPriority > 0) | ||
****************************************************************/ | ||
qos := util.MatchPodAndPodQOSSlice(pod, qosSlice) | ||
if qos == nil { | ||
klog.Infof("injection skipped: no podqos matched") | ||
return nil | ||
} | ||
|
||
if qos.Spec.ResourceQOS.CPUQOS == nil || | ||
qos.Spec.ResourceQOS.CPUQOS.CPUPriority == nil || | ||
*qos.Spec.ResourceQOS.CPUQOS.CPUPriority == 0 { | ||
klog.Infof("injection skipped: not a low CPUPriority pod, qos %s", qos.Name) | ||
return nil | ||
} | ||
for _, container := range pod.Spec.InitContainers { | ||
if container.Name == m.Config.QOSInitializer.InitContainerTemplate.Name { | ||
payall4u marked this conversation as resolved.
Show resolved
Hide resolved
|
||
klog.Infof("injection skipped: pod has initializerContainer already") | ||
return nil | ||
} | ||
} | ||
|
||
if m.Config.QOSInitializer.VolumeTemplate != nil { | ||
pod.Spec.Volumes = append(pod.Spec.Volumes, *m.Config.QOSInitializer.VolumeTemplate) | ||
for _, volume := range pod.Spec.Volumes { | ||
if volume.Name == m.Config.QOSInitializer.VolumeTemplate.Name { | ||
klog.Infof("injection skipped: pod has initializerVolume already") | ||
return nil | ||
} | ||
} | ||
|
||
klog.Infof("Injected QOSInitializer for Pod %s/%s", pod.Namespace, pod.Name) | ||
if m.Config.QOSInitializer.InitContainerTemplate != nil { | ||
pod.Spec.InitContainers = append(pod.Spec.InitContainers, *m.Config.QOSInitializer.InitContainerTemplate) | ||
} | ||
|
||
if m.Config.QOSInitializer.VolumeTemplate != nil { | ||
pod.Spec.Volumes = append(pod.Spec.Volumes, *m.Config.QOSInitializer.VolumeTemplate) | ||
} | ||
|
||
klog.Infof("mutating completed for pod %s/%s", pod.Namespace, pod.Name) | ||
|
||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,28 +100,36 @@ data: | |
kind: QOSConfig | ||
qosInitializer: | ||
enable: true | ||
selector: | ||
selector: | ||
matchLabels: | ||
app: nginx | ||
initContainerTemplate: | ||
name: crane-qos-initializer | ||
name: qos-initializer-container | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: name: qos-initializer is fine |
||
image: docker.io/gocrane/qos-init:v0.1.6 | ||
imagePullPolicy: IfNotPresent | ||
args: | ||
- "while ! grep -q gocrane.io/cpu-qos /etc/podinfo/annotations; do sleep 1; done; echo cpu qos setting competed;" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if it's just a shell call, do we still need qos-init image, can it be changed to pause? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, pause does not contain those shell command, ignore this |
||
command: | ||
- sh | ||
- -x | ||
- /qos-checking.sh | ||
- /bin/bash | ||
- -c | ||
resources: | ||
requests: | ||
cpu: 10m | ||
memory: 10Mi | ||
limits: | ||
cpu: 10m | ||
memory: 10Mi | ||
volumeMounts: | ||
- name: podinfo | ||
- name: qos-initializer-volume | ||
mountPath: /etc/podinfo | ||
volumeTemplate: | ||
name: podinfo | ||
name: qos-initializer-volume | ||
downwardAPI: | ||
items: | ||
- path: "annotations" | ||
fieldRef: | ||
fieldPath: metadata.annotations | ||
|
||
--- | ||
apiVersion: admissionregistration.k8s.io/v1 | ||
kind: MutatingWebhookConfiguration | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this to the crane import section