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

Improve Image Builder Jobs Label and Annotation #380

Merged
merged 7 commits into from
Apr 17, 2023
Merged

Improve Image Builder Jobs Label and Annotation #380

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
265b533
Add safe-to-evict annotation to image builder job. Improve labels on …
ariefrahmansyah Apr 17, 2023
e713369
Remove e2e's kserve cpu limits
ariefrahmansyah Apr 17, 2023
8800a90
Remove e2e's kserve statefulset as we already has the deployment
ariefrahmansyah Apr 17, 2023
a9e26d8
Fix rollout status command
ariefrahmansyah Apr 17, 2023
4c5a21d
Fix getting logs for kserve deployment
ariefrahmansyah Apr 17, 2023
485a995
Address review
ariefrahmansyah Apr 17, 2023
1f05241
Address review: Add comment on why we set safe-to-evict to false
ariefrahmansyah Apr 17, 2023
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: 2 additions & 2 deletions .github/workflows/merlin.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ on:
env:
ARTIFACT_RETENTION_DAYS: 7
DOCKER_BUILDKIT: 1
GO_VERSION: '1.20'
GO_VERSION: "1.20"

jobs:
create-version:
Expand Down Expand Up @@ -477,7 +477,7 @@ jobs:
echo "::endgroup::"

echo "::group::KServe log"
kubectl logs statefulset/kserve-controller-manager -n kserve -c manager
kubectl logs deployment/kserve-controller-manager -n kserve -c manager
echo "::endgroup::"

echo "::group::KServe events"
Expand Down
2 changes: 2 additions & 0 deletions api/cmd/api/setup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ func initImageBuilder(cfg *config.Config) (webserviceBuilder imagebuilder.ImageB
Tolerations: cfg.ImageBuilderConfig.Tolerations,
NodeSelectors: cfg.ImageBuilderConfig.NodeSelectors,
MaximumRetry: cfg.ImageBuilderConfig.MaximumRetry,
SafeToEvict: cfg.ImageBuilderConfig.SafeToEvict,

ClusterName: cfg.ImageBuilderConfig.ClusterName,
GcpProject: cfg.ImageBuilderConfig.GcpProject,
Expand All @@ -164,6 +165,7 @@ func initImageBuilder(cfg *config.Config) (webserviceBuilder imagebuilder.ImageB
Tolerations: cfg.ImageBuilderConfig.Tolerations,
NodeSelectors: cfg.ImageBuilderConfig.NodeSelectors,
MaximumRetry: cfg.ImageBuilderConfig.MaximumRetry,
SafeToEvict: cfg.ImageBuilderConfig.SafeToEvict,

ClusterName: cfg.ImageBuilderConfig.ClusterName,
GcpProject: cfg.ImageBuilderConfig.GcpProject,
Expand Down
1 change: 1 addition & 0 deletions api/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ type ImageBuilderConfig struct {
NodeSelectors DictEnv `envconfig:"IMG_BUILDER_NODE_SELECTORS"`
MaximumRetry int32 `envconfig:"IMG_BUILDER_MAX_RETRY" default:"3"`
K8sConfig mlpcluster.K8sConfig `envconfig:"IMG_BUILDER_K8S_CONFIG"`
SafeToEvict bool `envconfig:"IMG_BUILDER_SAFE_TO_EVICT" default:"false"`
}

type Tolerations []v1.Toleration
Expand Down
2 changes: 2 additions & 0 deletions api/pkg/imagebuilder/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ type Config struct {
NodeSelectors map[string]string
// Maximum number of retry of image builder job
MaximumRetry int32
// Value for cluster-autoscaler.kubernetes.io/safe-to-evict annotation
SafeToEvict bool

// Cluster Name
ClusterName string
Expand Down
16 changes: 12 additions & 4 deletions api/pkg/imagebuilder/imagebuilder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,12 @@ func (c *imageBuilder) createKanikoJobSpec(project mlp.Project, model *models.Mo
Component: models.ComponentImageBuilder,
Stream: project.Stream,
Team: project.Team,
Labels: models.MergeProjectVersionLabels(project.Labels, version.Labels),
}

annotations := make(map[string]string)
if !c.config.SafeToEvict {
annotations["cluster-autoscaler.kubernetes.io/safe-to-evict"] = fmt.Sprint(c.config.SafeToEvict)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Super nit: this can just be:

annotations["cluster-autoscaler.kubernetes.io/safe-to-evict"] = "false"

Also, perhaps we can leave an inline comment on why we add this annotation conditionally, linking to the doc?

}

baseImageTag, ok := c.config.BaseImages[version.PythonVersion]
Expand Down Expand Up @@ -411,9 +417,10 @@ func (c *imageBuilder) createKanikoJobSpec(project mlp.Project, model *models.Mo

job := &batchv1.Job{
ObjectMeta: metav1.ObjectMeta{
Name: kanikoPodName,
Namespace: c.config.BuildNamespace,
Labels: metadata.ToLabel(),
Name: kanikoPodName,
Namespace: c.config.BuildNamespace,
Labels: metadata.ToLabel(),
Annotations: annotations,
},
Spec: batchv1.JobSpec{
Completions: &jobCompletions,
Expand All @@ -422,7 +429,8 @@ func (c *imageBuilder) createKanikoJobSpec(project mlp.Project, model *models.Mo
ActiveDeadlineSeconds: &activeDeadlineSeconds,
Template: v1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Labels: metadata.ToLabel(),
Labels: metadata.ToLabel(),
Annotations: annotations,
},
Spec: v1.PodSpec{
// https://stackoverflow.com/questions/54091659/kubernetes-pods-disappear-after-failed-jobs
Expand Down
93 changes: 93 additions & 0 deletions api/pkg/imagebuilder/imagebuilder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ var (
ID: models.ID(1),
ArtifactURI: testArtifactURI,
PythonVersion: "3.10.*",
Labels: models.KV{
"test": "true",
},
}

timeout, _ = time.ParseDuration("10s")
Expand Down Expand Up @@ -246,6 +249,11 @@ func TestBuildImage(t *testing.T) {
"gojek.com/orchestrator": testOrchestratorName,
"gojek.com/stream": project.Stream,
"gojek.com/team": project.Team,
"sample": "true",
"test": "true",
},
Annotations: map[string]string{
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
},
},
Spec: batchv1.JobSpec{
Expand All @@ -262,6 +270,11 @@ func TestBuildImage(t *testing.T) {
"gojek.com/orchestrator": testOrchestratorName,
"gojek.com/stream": project.Stream,
"gojek.com/team": project.Team,
"sample": "true",
"test": "true",
},
Annotations: map[string]string{
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
},
},
Spec: v1.PodSpec{
Expand Down Expand Up @@ -346,6 +359,11 @@ func TestBuildImage(t *testing.T) {
"gojek.com/team": project.Team,
"gojek.com/environment": config.Environment,
"gojek.com/component": "image-builder",
"sample": "true",
"test": "true",
},
Annotations: map[string]string{
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
},
},
Spec: batchv1.JobSpec{
Expand All @@ -362,6 +380,11 @@ func TestBuildImage(t *testing.T) {
"gojek.com/team": project.Team,
"gojek.com/environment": config.Environment,
"gojek.com/component": "image-builder",
"sample": "true",
"test": "true",
},
Annotations: map[string]string{
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
},
},
Spec: v1.PodSpec{
Expand Down Expand Up @@ -424,6 +447,11 @@ func TestBuildImage(t *testing.T) {
"gojek.com/orchestrator": testOrchestratorName,
"gojek.com/stream": project.Stream,
"gojek.com/team": project.Team,
"sample": "true",
"test": "true",
},
Annotations: map[string]string{
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
},
},
Spec: batchv1.JobSpec{
Expand All @@ -440,6 +468,11 @@ func TestBuildImage(t *testing.T) {
"gojek.com/orchestrator": testOrchestratorName,
"gojek.com/stream": project.Stream,
"gojek.com/team": project.Team,
"sample": "true",
"test": "true",
},
Annotations: map[string]string{
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
},
},
Spec: v1.PodSpec{
Expand Down Expand Up @@ -552,6 +585,11 @@ func TestBuildImage(t *testing.T) {
"gojek.com/orchestrator": testOrchestratorName,
"gojek.com/stream": project.Stream,
"gojek.com/team": project.Team,
"sample": "true",
"test": "true",
},
Annotations: map[string]string{
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
},
},
Spec: batchv1.JobSpec{
Expand All @@ -568,6 +606,11 @@ func TestBuildImage(t *testing.T) {
"gojek.com/orchestrator": testOrchestratorName,
"gojek.com/stream": project.Stream,
"gojek.com/team": project.Team,
"sample": "true",
"test": "true",
},
Annotations: map[string]string{
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
},
},
Spec: v1.PodSpec{
Expand Down Expand Up @@ -690,6 +733,11 @@ func TestBuildImage(t *testing.T) {
"gojek.com/orchestrator": testOrchestratorName,
"gojek.com/stream": project.Stream,
"gojek.com/team": project.Team,
"sample": "true",
"test": "true",
},
Annotations: map[string]string{
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
},
},
Spec: batchv1.JobSpec{
Expand All @@ -706,6 +754,11 @@ func TestBuildImage(t *testing.T) {
"gojek.com/orchestrator": testOrchestratorName,
"gojek.com/stream": project.Stream,
"gojek.com/team": project.Team,
"sample": "true",
"test": "true",
},
Annotations: map[string]string{
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
},
},
Spec: v1.PodSpec{
Expand Down Expand Up @@ -822,6 +875,11 @@ func TestBuildImage(t *testing.T) {
"gojek.com/orchestrator": testOrchestratorName,
"gojek.com/stream": project.Stream,
"gojek.com/team": project.Team,
"sample": "true",
"test": "true",
},
Annotations: map[string]string{
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
},
},
Spec: batchv1.JobSpec{
Expand All @@ -838,6 +896,11 @@ func TestBuildImage(t *testing.T) {
"gojek.com/orchestrator": testOrchestratorName,
"gojek.com/stream": project.Stream,
"gojek.com/team": project.Team,
"sample": "true",
"test": "true",
},
Annotations: map[string]string{
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
},
},
Spec: v1.PodSpec{
Expand Down Expand Up @@ -921,6 +984,11 @@ func TestBuildImage(t *testing.T) {
"gojek.com/orchestrator": testOrchestratorName,
"gojek.com/stream": project.Stream,
"gojek.com/team": project.Team,
"sample": "true",
"test": "true",
},
Annotations: map[string]string{
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
},
},
Spec: batchv1.JobSpec{
Expand All @@ -937,6 +1005,11 @@ func TestBuildImage(t *testing.T) {
"gojek.com/orchestrator": testOrchestratorName,
"gojek.com/stream": project.Stream,
"gojek.com/team": project.Team,
"sample": "true",
"test": "true",
},
Annotations: map[string]string{
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
},
},
Spec: v1.PodSpec{
Expand Down Expand Up @@ -1023,6 +1096,11 @@ func TestBuildImage(t *testing.T) {
"gojek.com/orchestrator": testOrchestratorName,
"gojek.com/stream": project.Stream,
"gojek.com/team": project.Team,
"sample": "true",
"test": "true",
},
Annotations: map[string]string{
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
},
},
Spec: batchv1.JobSpec{
Expand All @@ -1039,6 +1117,11 @@ func TestBuildImage(t *testing.T) {
"gojek.com/orchestrator": testOrchestratorName,
"gojek.com/stream": project.Stream,
"gojek.com/team": project.Team,
"sample": "true",
"test": "true",
},
Annotations: map[string]string{
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
},
},
Spec: v1.PodSpec{
Expand Down Expand Up @@ -1113,6 +1196,11 @@ func TestBuildImage(t *testing.T) {
"gojek.com/orchestrator": testOrchestratorName,
"gojek.com/stream": project.Stream,
"gojek.com/team": project.Team,
"sample": "true",
"test": "true",
},
Annotations: map[string]string{
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
},
},
Spec: batchv1.JobSpec{
Expand All @@ -1129,6 +1217,11 @@ func TestBuildImage(t *testing.T) {
"gojek.com/orchestrator": testOrchestratorName,
"gojek.com/stream": project.Stream,
"gojek.com/team": project.Team,
"sample": "true",
"test": "true",
},
Annotations: map[string]string{
"cluster-autoscaler.kubernetes.io/safe-to-evict": "false",
},
},
Spec: v1.PodSpec{
Expand Down
2 changes: 2 additions & 0 deletions charts/merlin/templates/merlin-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,8 @@ spec:
- name: GOOGLE_APPLICATION_CREDENTIALS
value: /etc/gcp_service_account/service-account.json
{{- end }}
- name: IMG_BUILDER_SAFE_TO_EVICT
value: "{{ .Values.merlin.imageBuilder.safeToEvict }}"
volumeMounts:
- mountPath: /opt/config
name: config
Expand Down
7 changes: 4 additions & 3 deletions charts/merlin/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ merlin:

environment: dev
deploymentLabelPrefix: "gojek.com/"
pyfuncGRPCOptions : "{}"
pyfuncGRPCOptions: "{}"

loggerDestinationURL: "http://yourDestinationLogger"

Expand Down Expand Up @@ -82,6 +82,7 @@ merlin:
nodeSelectors: {}
maxRetry: 3
k8sConfig: ""
safeToEvict: false

gitlab:
baseURL: https://gitlab.com/
Expand Down Expand Up @@ -172,7 +173,7 @@ merlin:
keepAliveTime: 60s
keepAliveTimeout: 5s
kafka:
# brokers:
# brokers:
maxMessageSize: "1048588"

# Google service account used to access GCP's resources.
Expand Down Expand Up @@ -425,7 +426,7 @@ swagger:
externalPort: 8080

mlp:
environmentConfigSecret:
environmentConfigSecret:
name: ""
envKey: environment.yaml
imageBuilderKey: imageBuilderK8sConfig
26 changes: 15 additions & 11 deletions scripts/e2e/config/kserve/overlay.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,28 @@
apiVersion: apps/v1
kind: StatefulSet
metadata:
name: kserve-controller-manager
namespace: kserve
$patch: delete
krithika369 marked this conversation as resolved.
Show resolved Hide resolved
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: kserve-controller-manager
namespace: kserve
spec:
template:
spec:
containers:
- name: manager
image: kserve/kserve-controller:v0.9.0
resources:
limits:
cpu: 1
memory: 300Mi
requests:
cpu: 100m
memory: 64Mi

- name: manager
resources:
limits:
cpu: ~
krithika369 marked this conversation as resolved.
Show resolved Hide resolved
memory: 300Mi
requests:
cpu: 100m
memory: 64Mi
---

apiVersion: v1
kind: ConfigMap
metadata:
Expand Down
Loading