add failed state

[davixcky]

[daniel-de-vera]

@davixcky, this looks good to me, but I would extend the info we display in the other states as well.
I had this PR #130, but it looks like it won't get merged, so I would suggest adding the logic from below in this PR:

cli/internal/command/jobs/printers.go

Lines 94 to 112 in 220ffe1

	fmt.Fprintf(tw, "Job Name:\t%s\n", job.Name)
	fmt.Fprintf(tw, "Job Runner Group:\t%s\n", job.Spec.RunnerGroup)
	fmt.Fprintf(tw, "Status:\t%s\n", getJobStatus(job))
	if state := job.Status.Attempts[0].State; state != nil {
	switch {
	case state.Queued != nil:
	fmt.Fprintf(tw, "Message:\t%s\n", state.Queued.Message)
	case state.Running != nil:
	fmt.Fprintf(tw, "Runner Pod:\t%s/%s\n", state.Running.PodNamespace, state.Running.PodName)
	case state.Canceled != nil:
	fmt.Fprintf(tw, "Canceled By:\t%s\n", state.Canceled.CanceledBy)
	fmt.Fprintf(tw, "Message:\t%s\n", state.Canceled.Message)
	case state.Failed != nil:
	if state.Failed.ExitCode != nil {
	fmt.Fprintf(tw, "Exit Code:\t%d\n", *state.Failed.ExitCode)
	}
	fmt.Fprintf(tw, "Message:\t%s\n", state.Failed.Message)
	}
	}

WDYT?

[davixcky]

@davixcky, this looks good to me, but I would extend the info we display in the other states as well. I had this PR #130, but it looks like it won't get merged, so I would suggest adding the logic from below in this PR:

cli/internal/command/jobs/printers.go

Lines 94 to 112 in 220ffe1

	fmt.Fprintf(tw, "Job Name:\t%s\n", job.Name)
	fmt.Fprintf(tw, "Job Runner Group:\t%s\n", job.Spec.RunnerGroup)
	fmt.Fprintf(tw, "Status:\t%s\n", getJobStatus(job))
	if state := job.Status.Attempts[0].State; state != nil {
	switch {
	case state.Queued != nil:
	fmt.Fprintf(tw, "Message:\t%s\n", state.Queued.Message)
	case state.Running != nil:
	fmt.Fprintf(tw, "Runner Pod:\t%s/%s\n", state.Running.PodNamespace, state.Running.PodName)
	case state.Canceled != nil:
	fmt.Fprintf(tw, "Canceled By:\t%s\n", state.Canceled.CanceledBy)
	fmt.Fprintf(tw, "Message:\t%s\n", state.Canceled.Message)
	case state.Failed != nil:
	if state.Failed.ExitCode != nil {
	fmt.Fprintf(tw, "Exit Code:\t%d\n", *state.Failed.ExitCode)
	}
	fmt.Fprintf(tw, "Message:\t%s\n", state.Failed.Message)
	}
	}

WDYT?

Makes sense, updated

[daniel-de-vera]

LGTM

It seems there is a bug in the API, hold on merging this changes

[daniel-de-vera]

This doesn't look good:

$ go run ./cmd/signadot/ job get basic-ipmqgdi
Job Name:           basic-ipmqgdi
Job Runner Group:   basic
Status:             Running
Runner Pod:         26/10        <============
Environment:        baseline
Created At:         about a minute ago
Started At:         about a minute ago
Dashboard URL:      https://app.signadot.com/testing/jobs/basic-ipmqgdi

Artifacts
No artifacts

I checked the API is returning invalid info:

$ sdcurl -e dev /api/v2/orgs/ddvcorp/jobs/basic-wx31lgq
GET http://localhost:8080/api/v2/orgs/ddvcorp/jobs/basic-wx31lgq

200 OK in 7.263833ms

{
  "name": "basic-wx31lgq",
  "createdAt": "2024-06-17T14:45:14Z",
  "deletedAt": "",
  "spec": {
    "namePrefix": "basic",
    "runnerGroup": "basic",
    "script": "#!/bin/bash\n\necho \"Hello this is the demo meeting\"\nstart=`date +%s`\n\nx=1\nwhile [ $x -le 30 ]\ndo\n  echo \"Welcome $x times (env TEST=$TEST)\"\n  x=$(( $x + 1 ))\n  echo \"Error $x: retrying\" 1\u003e\u00262\n  sleep 1\ndone\necho \"This is an artifact\" \u003e aaa.txt\necho \"We are done!\"\n\nend=`date +%s`\nruntime=$((end-start))\necho $runtime\n",
    "routingContext": null,
    "uploadArtifact": [
      {
        "path": "aaa.txt",
        "meta": {
          "format": "text"
        }
      }
    ]
  },
  "status": {
    "attempts": [
      {
        "id": 0,
        "createdAt": "2024-06-17T14:45:14Z",
        "sentToClusterBy": "2024-06-17T14:45:16Z",
        "startedAt": "2024-06-17T14:45:16Z",
        "phase": "running",
        "state": {
          "running": {
            "jobExecutorAddr": "10.244.26.50:3773",
            "podName": "10",             <========================
            "podNamespace": "26"         <========================
          }
        },
        "executionCount": 1,
        "tries": [
          {
            "seqNumber": 34
          }
        ]
      }
    ],
    "seqNum": 34
  }
}

Let's check what's going on before merging this one (maybe those fields are not intended to be there)

[daniel-de-vera]

The above issue is related to https://github.com/signadot/signadot/pull/4574.
We should discuss how to approach this.

[scott-cotton]

(maybe those fields are not intended to be there)

They are intended to be there, so that a user can access to the pod via kubectl.

I missed the interaction with the DNS caching fix though.

[daniel-de-vera]

(maybe those fields are not intended to be there)

They are intended to be there, so that a user can access to the pod via kubectl.

I missed the interaction with the DNS caching fix though.

I guess the right thing to do here would be to report that in the job CRD, wdty @scott-cotton?

[scott-cotton]

(maybe those fields are not intended to be there)

They are intended to be there, so that a user can access to the pod via kubectl.
I missed the interaction with the DNS caching fix though.

I guess the right thing to do here would be to report that in the job CRD, wdty @scott-cotton?

Feels less accessible if not available via the saas resource.

[daniel-de-vera]

Feels less accessible if not available via the saas resource.

I'm proposing to keep it in the SaaS resource, but instead of parsing that info out of the JobExecutionAddr from job CRD, read it from explicit fields.

[scott-cotton]

Feels less accessible if not available via the saas resource.

I'm proposing to keep it in the SaaS resource, but instead of parsing that info out of the JobExecutionAddr from job CRD, read it from explicit fields.

yes, that sounds right to me.

[daniel-de-vera]

yes, that sounds right to me.

I can make this change, after the planning meeting.

[scott-cotton]

yes, that sounds right to me.

I can make this change, after the planning meeting.

(but there is also the question of operator compatibility)

[daniel-de-vera]

yes, that sounds right to me.

I can make this change, after the planning meeting.

(but there is also the question of operator compatibility)

Here is a PR that fixes the issue: https://github.com/signadot/signadot/pull/4575
No changes in the operator were required.

[daniel-de-vera]

Being fixed the API issue, LGTM