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

feat: deprecations metric #13735

Merged
merged 1 commit into from
Oct 11, 2024
Merged
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
1 change: 1 addition & 0 deletions .spelling
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ PVCs
Peixuan
Ploomber
Postgres
PriorityClass
RCs
Roadmap
RoleBinding
Expand Down
2 changes: 1 addition & 1 deletion cmd/argo/commands/cron/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func CreateCronWorkflows(ctx context.Context, filePaths []string, cliOpts *cliCr
if err != nil {
return fmt.Errorf("Failed to create cron workflow: %v", err)
}
fmt.Print(getCronWorkflowGet(created))
fmt.Print(getCronWorkflowGet(ctx, created))
}
return nil
}
2 changes: 1 addition & 1 deletion cmd/argo/commands/cron/get.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func NewGetCommand() *cobra.Command {
if err != nil {
return err
}
printCronWorkflow(cronWf, output.String())
printCronWorkflow(ctx, cronWf, output.String())
}
return nil
},
Expand Down
7 changes: 4 additions & 3 deletions cmd/argo/commands/cron/list.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package cron

import (
"context"
"fmt"
"os"
"text/tabwriter"
Expand Down Expand Up @@ -53,7 +54,7 @@ func NewListCommand() *cobra.Command {
}
switch listArgs.output.String() {
case "", "wide":
printTable(cronWfList.Items, &listArgs)
printTable(ctx, cronWfList.Items, &listArgs)
case "name":
for _, cronWf := range cronWfList.Items {
fmt.Println(cronWf.ObjectMeta.Name)
Expand All @@ -70,7 +71,7 @@ func NewListCommand() *cobra.Command {
return command
}

func printTable(wfList []wfv1.CronWorkflow, listArgs *listFlags) {
func printTable(ctx context.Context, wfList []wfv1.CronWorkflow, listArgs *listFlags) {
w := tabwriter.NewWriter(os.Stdout, 0, 0, 3, ' ', 0)
if listArgs.allNamespaces {
_, _ = fmt.Fprint(w, "NAMESPACE\t")
Expand All @@ -88,7 +89,7 @@ func printTable(wfList []wfv1.CronWorkflow, listArgs *listFlags) {
cleanLastScheduledTime = "N/A"
}
var cleanNextScheduledTime string
if next, err := GetNextRuntime(&cwf); err == nil {
if next, err := GetNextRuntime(ctx, &cwf); err == nil {
cleanNextScheduledTime = humanize.RelativeDurationShort(next, time.Now())
} else {
cleanNextScheduledTime = "N/A"
Expand Down
2 changes: 1 addition & 1 deletion cmd/argo/commands/cron/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ func updateCronWorkflows(ctx context.Context, filePaths []string, cliOpts *cliUp
if err != nil {
return fmt.Errorf("Failed to update workflow template: %v", err)
}
fmt.Print(getCronWorkflowGet(updated))
fmt.Print(getCronWorkflowGet(ctx, updated))
}
return nil
}
13 changes: 7 additions & 6 deletions cmd/argo/commands/cron/util.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package cron

import (
"context"
"encoding/json"
"fmt"
"log"
Expand All @@ -22,10 +23,10 @@ import (

// GetNextRuntime returns the next time the workflow should run in local time. It assumes the workflow-controller is in
// UTC, but nevertheless returns the time in the local timezone.
func GetNextRuntime(cwf *v1alpha1.CronWorkflow) (time.Time, error) {
func GetNextRuntime(ctx context.Context, cwf *v1alpha1.CronWorkflow) (time.Time, error) {
var nextRunTime time.Time
now := time.Now().UTC()
for _, schedule := range cwf.Spec.GetSchedulesWithTimezone() {
for _, schedule := range cwf.Spec.GetSchedulesWithTimezone(ctx) {
cronSchedule, err := cron.ParseStandard(schedule)
if err != nil {
return time.Time{}, err
Expand Down Expand Up @@ -77,7 +78,7 @@ func unmarshalCronWorkflows(wfBytes []byte, strict bool) []wfv1.CronWorkflow {
return nil
}

func printCronWorkflow(wf *wfv1.CronWorkflow, outFmt string) {
func printCronWorkflow(ctx context.Context, wf *wfv1.CronWorkflow, outFmt string) {
switch outFmt {
case "name":
fmt.Println(wf.ObjectMeta.Name)
Expand All @@ -88,13 +89,13 @@ func printCronWorkflow(wf *wfv1.CronWorkflow, outFmt string) {
outBytes, _ := yaml.Marshal(wf)
fmt.Print(string(outBytes))
case "wide", "":
fmt.Print(getCronWorkflowGet(wf))
fmt.Print(getCronWorkflowGet(ctx, wf))
default:
log.Fatalf("Unknown output format: %s", outFmt)
}
}

func getCronWorkflowGet(cwf *wfv1.CronWorkflow) string {
func getCronWorkflowGet(ctx context.Context, cwf *wfv1.CronWorkflow) string {
const fmtStr = "%-30s %v\n"

out := ""
Expand All @@ -116,7 +117,7 @@ func getCronWorkflowGet(cwf *wfv1.CronWorkflow) string {
out += fmt.Sprintf(fmtStr, "LastScheduledTime:", humanize.Timestamp(cwf.Status.LastScheduledTime.Time))
}

next, err := GetNextRuntime(cwf)
next, err := GetNextRuntime(ctx, cwf)
if err == nil {
out += fmt.Sprintf(fmtStr, "NextScheduledTime:", humanize.Timestamp(next)+" (assumes workflow-controller is in UTC)")
}
Expand Down
7 changes: 4 additions & 3 deletions cmd/argo/commands/cron/util_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package cron

import (
"context"
"testing"
"time"

Expand Down Expand Up @@ -56,13 +57,13 @@ Conditions:

func TestPrintCronWorkflow(t *testing.T) {
var cronWf = v1alpha1.MustUnmarshalCronWorkflow(invalidCwf)
out := getCronWorkflowGet(cronWf)
out := getCronWorkflowGet(context.Background(), cronWf)
assert.Contains(t, out, expectedOut)
}

func TestNextRuntime(t *testing.T) {
var cronWf = v1alpha1.MustUnmarshalCronWorkflow(invalidCwf)
next, err := GetNextRuntime(cronWf)
next, err := GetNextRuntime(context.Background(), cronWf)
require.NoError(t, err)
assert.LessOrEqual(t, next.Unix(), time.Now().Add(1*time.Minute).Unix())
assert.Greater(t, next.Unix(), time.Now().Unix())
Expand Down Expand Up @@ -94,7 +95,7 @@ spec:

func TestNextRuntimeWithMultipleSchedules(t *testing.T) {
var cronWf = v1alpha1.MustUnmarshalCronWorkflow(cronMultipleSchedules)
next, err := GetNextRuntime(cronWf)
next, err := GetNextRuntime(context.Background(), cronWf)
require.NoError(t, err)
assert.LessOrEqual(t, next.Unix(), time.Now().Add(1*time.Minute).Unix())
assert.Greater(t, next.Unix(), time.Now().Unix())
Expand Down
19 changes: 18 additions & 1 deletion config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
"net/url"
"time"

metricsdk "go.opentelemetry.io/otel/sdk/metric"
"go.opentelemetry.io/otel/sdk/metric/metricdata"
apiv1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
Expand Down Expand Up @@ -288,13 +290,28 @@ type MetricsConfig struct {
Temporality MetricsTemporality `json:"temporality,omitempty"`
}

func (mc MetricsConfig) GetSecure(defaultValue bool) bool {
func (mc *MetricsConfig) GetSecure(defaultValue bool) bool {
if mc.Secure != nil {
return *mc.Secure
}
return defaultValue
}

func (mc *MetricsConfig) GetTemporality() metricsdk.TemporalitySelector {
switch mc.Temporality {
case MetricsTemporalityCumulative:
return func(metricsdk.InstrumentKind) metricdata.Temporality {
return metricdata.CumulativeTemporality
}
case MetricsTemporalityDelta:
return func(metricsdk.InstrumentKind) metricdata.Temporality {
return metricdata.DeltaTemporality
}
default:
return metricsdk.DefaultTemporalitySelector
}
}

type WorkflowRestrictions struct {
TemplateReferencing TemplateReferencing `json:"templateReferencing,omitempty"`
}
Expand Down
75 changes: 75 additions & 0 deletions docs/deprecations.md
Copy link
Member

@agilgur5 agilgur5 Oct 18, 2024

Choose a reason for hiding this comment

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

Similarly to above, I would put all the H2 headings here a under a 3.6 version heading so that the upgrading guide could reference it per version. With no versions on this page, it's hard to understand when something was deprecated, and some of these also reference features that are only available in newer versions

As we have already had users attempt to use 3.5 and 3.6 features in older versions when there were no version annotations (#13645 as one example), I could definitely see users reading this, seeing the deprecation, trying the new syntax, and then getting a syntax error and raising an issue.

Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
# Deprecations

Sometimes a feature of Argo Workflows is deprecated.
You should stop using a deprecated feature as it may be removed in a future minor or major release of Argo Workflows.

To determine if you are using a deprecated feature the [`deprecated_feature`](metrics.md#deprecated_feature) metric can help.
Copy link
Member

Choose a reason for hiding this comment

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

It would definitely be good to have a version annotation for this metric too, as people will see this page, try to use it, and not realize that it's not available in older versions, and perhaps even think they're in the clear because of that.

This metric will go up for each use of a deprecated feature by the workflow controller.
This means it may go up once or many times for a single event.
If the number is going up the feature is still in use by your system.
If the metric is not present or no longer increasing are no longer using the monitored deprecated features.

## `cronworkflow schedule`
Copy link
Member

Choose a reason for hiding this comment

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

I would name these for their respective features so that they are easily correlated, and then perhaps have a "> cronworkflow schedule attribute" to indicate that this is what the metric displays. I think it's difficult to tell that this is referring to those specific names unless you have specifically came from the metric's page and read it in detail.

This page isn't specific to the metric though, it's just named "Deprecations" in general as well, so it can reference it but should be its own independent page as well

Copy link
Member

Choose a reason for hiding this comment

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

It would also be good to link to the feature page for each of these which will have more details and examples


The spec field `schedule` which takes a single value is replaced by `schedules` which takes a list.
To update this replace the `schedule` with `schedules` as in the following example
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
To update this replace the `schedule` with `schedules` as in the following example
To update this replace the `schedule` with `schedules` as in the following example:

Colons are also missing in prior to all the examples, which are conventionally (and gramatically) used when describing an example in the docs


```yaml
spec:
schedule: "30 1 * * *"
```
is replaced with
```yaml
spec:
schedules:
- "30 1 * * *"
Comment on lines +19 to +27
Copy link
Member

Choose a reason for hiding this comment

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

These examples would likely be easier to understand quickly with a ```diff, i.e.:

-   schedule: "30 1 * * *"
+   schedules:
+     - "30 1 * * *"

```
## `synchronization mutex`

The synchronization field `mutex` which takes a single value is replaced by `mutexes` which takes a list.
To update this replace `mutex` with `mutexes` as in the following example

```yaml
synchronization:
mutex:
name: foobar
```

is replaced with

```yaml
synchronization:
mutexes:
- name: foobar
```

## `synchronization semaphore`

The synchronization field `semaphore` which takes a single value is replaced by `semaphores` which takes a list.
To update this replace `semaphore` with `semaphores` as in the following example

```yaml
synchronization:
semaphore:
configMapKeyRef:
name: my-config
key: workflow
```

is replaced with

```yaml
synchronization:
semaphores:
- configMapKeyRef:
name: my-config
key: workflow
```

## `workflow podpriority`

The Workflow spec field `podPriority` which takes a numeric value is deprecated and `podPriorityClassName` should be used instead.
To update this you will need a [PriorityClass](https://kubernetes.io/docs/concepts/scheduling-eviction/pod-priority-preemption/#priorityclass) in your cluster and refer to that using `podPriorityClassName`.
17 changes: 17 additions & 0 deletions docs/metrics.md
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,23 @@ Suppressed runs due to `concurrencyPolicy: Forbid` will not be counted.
| `name` | ⚠️ The name of the CronWorkflow |
| `namespace` | The namespace of the CronWorkflow |

#### `deprecated_feature`

A counter which goes up when a feature which is [deprecated](deprecations.md) is used.
🚨 This counter may go up much more than once for a single use of the feature.

| attribute | explanation |
|-------------|---------------------------------------------|
| `feature` | The name of the feature used |
| `namespace` | The namespace of the item using the feature |

`feature` will be one of:

- [`cronworkflow schedule`](deprecations.md#cronworkflow_schedule)
- [`synchronization mutex`](deprecations.md#synchronization_mutex)
- [`synchronization semaphore`](deprecations.md#synchronization_semaphore)
- [`workflow podpriority`](deprecations.md#workflow_podpriority)

#### `gauge`

A gauge of the number of workflows currently in the cluster in each phase. The `Running` count does not mean that a workflows pods are running, just that the controller has scheduled them. A workflow can be stuck in `Running` with pending pods for a long time.
Expand Down
1 change: 1 addition & 0 deletions docs/upgrading.md
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ The following are new metrics:

* `cronworkflows_concurrencypolicy_triggered`
* `cronworkflows_triggered_total`
* `deprecated_feature`
* `is_leader`
* `k8s_request_duration`
* `pod_pending_count`
Expand Down
1 change: 1 addition & 0 deletions mkdocs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ nav:
- offloading-large-workflows.md
- workflow-archive.md
- metrics.md
- deprecations.md
Copy link
Member

@agilgur5 agilgur5 Oct 11, 2024

Choose a reason for hiding this comment

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

I'd put this next to upgrading.md and link to it from there too.

Same as the "New features" page we discussed in the Contributor Meeting, having it per version and matching up with the upgrading guide will make it easier to follow. EDIT: See also #13739 (comment)

Although arguably deprecations should be part of the upgrading guide.

Copy link
Member

Choose a reason for hiding this comment

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

Although arguably deprecations should be part of the upgrading guide.

I see this has been done in #13757 now

- workflow-executors.md
- workflow-restrictions.md
- sidecar-injection.md
Expand Down
2 changes: 1 addition & 1 deletion pkg/apiclient/offline-cron-workflow-service-client.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ type OfflineCronWorkflowServiceClient struct {
var _ cronworkflow.CronWorkflowServiceClient = &OfflineCronWorkflowServiceClient{}

func (o OfflineCronWorkflowServiceClient) LintCronWorkflow(ctx context.Context, req *cronworkflow.LintCronWorkflowRequest, _ ...grpc.CallOption) (*v1alpha1.CronWorkflow, error) {
err := validate.ValidateCronWorkflow(o.namespacedWorkflowTemplateGetterMap.GetNamespaceGetter(req.Namespace), o.clusterWorkflowTemplateGetter, req.CronWorkflow)
err := validate.ValidateCronWorkflow(ctx, o.namespacedWorkflowTemplateGetterMap.GetNamespaceGetter(req.Namespace), o.clusterWorkflowTemplateGetter, req.CronWorkflow)
if err != nil {
return nil, err
}
Expand Down
13 changes: 8 additions & 5 deletions pkg/apis/workflow/v1alpha1/cron_workflow_types.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package v1alpha1

import (
"context"
"strings"

v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"

"github.com/argoproj/argo-workflows/v3/pkg/apis/workflow"
"github.com/argoproj/argo-workflows/v3/util/deprecation"
)

// CronWorkflow is the definition of a scheduled workflow resource
Expand Down Expand Up @@ -169,24 +171,25 @@ func (c *CronWorkflowSpec) getScheduleString(withTimezone bool) string {

// GetSchedulesWithTimezone returns all schedules configured for the CronWorkflow with a timezone. It handles
// both Spec.Schedules and Spec.Schedule for backwards compatibility
func (c *CronWorkflowSpec) GetSchedulesWithTimezone() []string {
return c.getSchedules(true)
func (c *CronWorkflowSpec) GetSchedulesWithTimezone(ctx context.Context) []string {
return c.getSchedules(ctx, true)
}

// GetSchedules returns all schedules configured for the CronWorkflow. It handles both Spec.Schedules
// and Spec.Schedule for backwards compatibility
func (c *CronWorkflowSpec) GetSchedules() []string {
return c.getSchedules(false)
func (c *CronWorkflowSpec) GetSchedules(ctx context.Context) []string {
return c.getSchedules(ctx, false)
}

func (c *CronWorkflowSpec) getSchedules(withTimezone bool) []string {
func (c *CronWorkflowSpec) getSchedules(ctx context.Context, withTimezone bool) []string {
var schedules []string
if c.Schedule != "" {
schedule := c.Schedule
if withTimezone {
schedule = c.withTimezone(c.Schedule)
}
schedules = append(schedules, schedule)
deprecation.Record(ctx, deprecation.Schedule)
} else {
schedules = make([]string, len(c.Schedules))
for i, schedule := range c.Schedules {
Expand Down
Loading
Loading