Skip to content
This repository has been archived by the owner on Nov 15, 2022. It is now read-only.

implement jobs #116

Closed
wants to merge 2 commits into from
Closed

implement jobs #116

wants to merge 2 commits into from

Conversation

concaf
Copy link
Collaborator

@concaf concaf commented Jul 5, 2017

No description provided.

@cdrage
Copy link
Collaborator

cdrage commented Jul 5, 2017

@containscafeine WIP?

ports:
- port: 8080
jobs:
- containers:
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is so confusing :(

So essentially we have services using containers as a root key, but jobs has containers within it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, the containers above is for your application, and the one in jobs is for your jobs.
How do you suggest we do this? I think this correlates with the ongoing discussions in #110.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@containscafeine I'm opening an issue now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tracked at #119

Copy link
Member

Choose a reason for hiding this comment

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

I don't see any problems with jobs having containers? @cdrage Why do you think its confusing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because we have two different formats. Containers as a subkey and containers as a root key. Depending on whether it's a service or any other kind of kind such as jobs, daemonset, statefulset, etc.

Copy link
Member

Choose a reason for hiding this comment

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

I still don't see it :-(

One is containers for the application (Deployment) the second one if for jobs.
Jobs has also containers, its completely different set of containers from the top level containers.

@concaf
Copy link
Collaborator Author

concaf commented Jul 5, 2017

@cdrage yep, yet to add a couple of tests, docs, etc.

for index, job := range app.Jobs {
// populate job name if not present
if job.Name == "" {
app.Jobs[index].Name = "job-" + strconv.Itoa(index+1)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't add 1 to index, even Kubernetes starts the indexing from 0, e.g. in case of StatefulSets (TIL) :P

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@kedge-bot
Copy link
Collaborator

@containscafeine, thank you for the pull request! We'll ping some people to review your PR. @surajssd, @kadel and @cdrage, please review this.

@concaf concaf self-assigned this Jul 12, 2017
pkg/spec/spec.go Outdated
@@ -70,6 +72,12 @@ type ConfigMapMod struct {
Data map[string]string `json:"data,omitempty"`
}

type JobSpecMod struct {
Name string `json:"name,omitempty"`
api.PodSpec `json:",inline"`
Copy link
Member

Choose a reason for hiding this comment

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

why is podspec here? Is it already in jobspec?

pkg/spec/spec.go Outdated

// overwrite containers from PodSpec
Containers []Container `json:"containers,omitempty"`

api_v1.PodSpec `json:",inline"`
ext_v1beta1.DeploymentSpec `json:",inline"`
batch.JobSpec `json:",inline"`
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is shouldn't be here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed

@kadel
Copy link
Member

kadel commented Jul 13, 2017

If its not working If I want to put use containers in JobSpec.

name: httpd
containers:
- image: centos/httpd
services:
- name: httpd
  type: NodePort
  ports:
  - port: 8080
jobs:
- name: myJob
  parallelism: 3
  activeDeadlineSeconds: 2
  template:
    metadata:
      name: pi
    spec:
      containers:
      - name: pi
        image: perl
        command: ["perl",  "-Mbignum=bpi", "-wle", "print bpi(2000)"]
      restartPolicy: Never
      activeDeadlineSeconds: 1

Generated Job is completly missing containers section and nothing from template.spec is propaged .

apiVersion: batch/v1
kind: Job
metadata:
  creationTimestamp: null
  labels:
    app: httpd
  name: myJob
spec:
  parallelism: 3
  template:
    metadata:
      creationTimestamp: null
      name: pi
    spec:
      containers: null
      serviceAccountName: ""
      volumes: null
status: {}

@surajssd
Copy link
Member

surajssd commented Jul 13, 2017

Is this WIP or review-needed PR? Because I don't think we have finalized on the way the jobs should be defined?

@concaf
Copy link
Collaborator Author

concaf commented Jul 14, 2017

@surajssd this is still WIP, I am waiting on the discussion at #116 (comment) before moving on.
The code is not being reviewed, it's only the examples and how to place jobs in spec.go.

@concaf
Copy link
Collaborator Author

concaf commented Aug 7, 2017

Refactored the existing code due to the client-go version bump, will now refactor the spec part.

@concaf concaf force-pushed the implement_jobs branch 3 times, most recently from 4d1785b to d63b913 Compare August 11, 2017 07:54
@concaf
Copy link
Collaborator Author

concaf commented Aug 16, 2017

  • add comments
  • fix tests
  • add docs
  • root level ActiveDeadlineSeconds will set only for Jobs, and not for Pods, for that you need to take the longer approach
    ping @kadel @surajssd PTAL at the approach, will make further modifications once the approach LGTY.

@concaf
Copy link
Collaborator Author

concaf commented Aug 16, 2017

Now starting to move interface methods related bits to package spec in a separate PR

@concaf
Copy link
Collaborator Author

concaf commented Sep 12, 2017

Deprecating in favor of #253

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants