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

feature(#580): adds traffic and tags information to revision list #581

Merged
merged 1 commit into from
Jan 10, 2020
Merged

feature(#580): adds traffic and tags information to revision list #581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 11 additions & 0 deletions pkg/kn/commands/revision/human_readable_flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,18 @@ import (
servingv1alpha1 "knative.dev/serving/pkg/apis/serving/v1alpha1"
)

const (
RevisionTrafficAnnotation = "client.knative.dev/traffic"
RevisionTagsAnnotation = "client.knative.dev/tags"
)

// RevisionListHandlers adds print handlers for revision list command
func RevisionListHandlers(h hprinters.PrintHandler) {
RevisionColumnDefinitions := []metav1beta1.TableColumnDefinition{
{Name: "Name", Type: "string", Description: "Name of the revision.", Priority: 1},
{Name: "Service", Type: "string", Description: "Name of the Knative service.", Priority: 1},
{Name: "Traffic", Type: "string", Description: "Percentage of traffic assigned to this revision.", Priority: 1},
{Name: "Tags", Type: "string", Description: "Set of tags assigned to this revision.", Priority: 1},
{Name: "Generation", Type: "string", Description: "Generation of the revision", Priority: 1},
{Name: "Age", Type: "string", Description: "Age of the revision.", Priority: 1},
{Name: "Conditions", Type: "string", Description: "Conditions describing statuses of the revision.", Priority: 1},
Expand Down Expand Up @@ -57,6 +64,8 @@ func printRevisionList(revisionList *servingv1alpha1.RevisionList, options hprin
func printRevision(revision *servingv1alpha1.Revision, options hprinters.PrintOptions) ([]metav1beta1.TableRow, error) {
name := revision.Name
service := revision.Labels[serving.ServiceLabelKey]
traffic := revision.Annotations[RevisionTrafficAnnotation]
tags := revision.Annotations[RevisionTagsAnnotation]
generation := revision.Labels[serving.ConfigurationGenerationLabelKey]
age := commands.TranslateTimestampSince(revision.CreationTimestamp)
conditions := commands.ConditionsValue(revision.Status.Conditions)
Expand All @@ -68,6 +77,8 @@ func printRevision(revision *servingv1alpha1.Revision, options hprinters.PrintOp
row.Cells = append(row.Cells,
name,
service,
traffic,
tags,
generation,
age,
conditions,
Expand Down
19 changes: 19 additions & 0 deletions pkg/kn/commands/revision/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"fmt"
"sort"
"strconv"
"strings"

"knative.dev/serving/pkg/apis/serving"

Expand Down Expand Up @@ -85,6 +86,23 @@ func NewRevisionListCommand(p *commands.KnParams) *cobra.Command {
}
}

// Add revision/service info as annotations: traffic and tags
var service *v1alpha1.Service
var serviceName string
for _, revision := range revisionList.Items {
if serviceName == "" || revision.Labels[serving.ServiceLabelKey] != serviceName {
Copy link
Contributor

Choose a reason for hiding this comment

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

(just working on an followup PR, but wanna leave some extra comments): It is by no means guaranteed that revisions from the list are sorted according to service. Better store in a map for latter referral.

Please don't fix I'm on it.

serviceName = revision.Labels[serving.ServiceLabelKey]
service, err = client.GetService(serviceName)
if err != nil {
return err
}
}

traffic, tags := trafficForRevision(revision.Name, service)
revision.Annotations[RevisionTrafficAnnotation] = fmt.Sprintf("%d%%", traffic)
revision.Annotations[RevisionTagsAnnotation] = strings.Join(tags, ",")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid modifying the revision here by e.g. using a map keyed by revision name ? Imagine if someone later would introduce such annotations for real (so that you would override it here). Also 'misusing' an object by writing to it in a pure read-only use case is not architectural sound.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought of that, however, the revision is not modified permanently. Only temporarily to hold values that frankly belongs to it. These modified revision objects are in memory and are never seen by 'anyone' else other than the user that invoked the command.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we avoid modifying the revision here by

+1

We can use trafficForRevision in printRevision to fetch the traffic and tags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Read my comment @navidshaikh it does not modify the revision except in memory which makes the rest of the code super clean

Copy link
Collaborator

@navidshaikh navidshaikh Dec 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Sure! I think this information doesn't belong to Revision and we shouldn't piggyback this data onto Revision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! I think this information doesn't belong to Revision and we shouldn't piggyback this data onto Revision.

It does at least when using the revision list. See why I opened this issue in the first place. It comes from the need to see this information and not having to have to show the details of the service and parsing the out to find the data. This is my feedback from real usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Imagine someone would print out the full object somewhere in between, like in a kn revision list --verbose or something. You will see artifical annotations here).

kn revision list --verbose call does not exist. If we decide to do it we can easily filter out the annotations or better flag them as such. Which I'd be happy to also do here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree since annotations are meant to do things like that. However, I am happy to hear suggestions on how else to do it?

That's where I disagree, i.e. that annotations are used for volatile information which never gets persisted ;-). But maybe we could at least agree that listing revisions is a read-only operation and could be implemented with immutable objects that don't allow mutating operations? Why should we break that without a need ?

kn revision list --verbose call does not exist. If we decide to do it we can easily filter out the annotations or better flag them as such. Which I'd be happy to also do here.

That's not really the point, whether this feature exists or not as its just an illustrative example. When you say one can easily add a filter, that is a bad smell, too, to use another hack to fix the previous hack.

Not sure whether I could convince you that keeping certain architectural constraints even when they are not enforced today is a good thing. So let's merge them at let me make an alternative suggestion in the next PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure whether I could convince you that keeping certain architectural constraints even when they are not enforced today is a good thing.

That's not the part that I disagree with.

That's where I disagree, i.e. that annotations are used for volatile information which never gets persisted ;-).

Yup, that's the conundrum. It's the use of annotations. For me, I feel that this is a good use since the attributes I am adding traffic and tags are not really "features" of the Service nor the Revision but of both. There are synthesized from and belong to the association between Service and Revision, in my view.

So let's merge them at let me make an alternative suggestion in the next PR.

OK cool. Look forward to it. Hopefully it's not a significant change since that was where I was headed until this approach. But maybe I am missing some easy trick... Happy to learn.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK cool. Look forward to it. Hopefully it's not a significant change since that was where I was headed until this approach. But maybe I am missing some easy trick... Happy to learn.

I have to apologize a bit :) Ideally I would use classical "view object" used for printing. But the printer framework we are using insist on runtime.Object. To implement those is even more hackish than adding to the annotations (I already was there before I returned back to your approach, which definitely is the better in this particular case). So I suggest indeed to stick to this technique for tunneling purposes (if we would have the printing of a list in our hands, its trivial to change with a revisionInfo object containing the revision + tags,traffic from the service).

However, my point is illustrated quite nicely what happens at the moment, when you are doing a kn revision list -o yaml. This includes, in YAML, the temporary annotations. This is incorrect as these annotations are not part of the revision. We can fix that, but in this initial PR it was overlooked. Something similar can happen in the future, too. Hopefully not, but anybody touching this code needs to be aware of this particular usage of volatile information attached to an otherwise persisted object.

So, there is no easy trick. Life is complicated ;-)

}

// sort revisionList by configuration generation key
sort.SliceStable(revisionList.Items, func(i, j int) bool {
a := revisionList.Items[i]
Expand All @@ -110,6 +128,7 @@ func NewRevisionListCommand(p *commands.KnParams) *cobra.Command {
if err != nil {
return err
}

err = printer.PrintObj(revisionList, cmd.OutOrStdout())
if err != nil {
return err
Expand Down
3 changes: 2 additions & 1 deletion pkg/kn/commands/revision/list_flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (
type RevisionListFlags struct {
GenericPrintFlags *genericclioptions.PrintFlags
HumanReadableFlags *commands.HumanPrintFlags
ServiceRefFlags ServiceReferenceFlags
ServiceRefFlags *ServiceReferenceFlags
}

// AllowedFormats is the list of formats in which data can be displayed
Expand Down Expand Up @@ -70,6 +70,7 @@ func NewRevisionListFlags() *RevisionListFlags {
return &RevisionListFlags{
GenericPrintFlags: genericclioptions.NewPrintFlags(""),
HumanReadableFlags: commands.NewHumanPrintFlags(),
ServiceRefFlags: &ServiceReferenceFlags{},
}
}

Expand Down
34 changes: 19 additions & 15 deletions pkg/kn/commands/revision/list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import (
"knative.dev/client/pkg/util"
)

var revisionListHeader = []string{"NAME", "SERVICE", "GENERATION", "AGE", "CONDITIONS", "READY", "REASON"}
var revisionListHeader = []string{"NAME", "SERVICE", "TRAFFIC", "TAGS", "GENERATION", "AGE", "CONDITIONS", "READY", "REASON"}

func fakeRevisionList(args []string, response *v1alpha1.RevisionList) (action client_testing.Action, output []string, err error) {
knParams := &commands.KnParams{}
Expand Down Expand Up @@ -71,13 +71,13 @@ func TestRevisionListEmptyByName(t *testing.T) {
}

func TestRevisionListDefaultOutput(t *testing.T) {
revision1 := createMockRevisionWithParams("foo-abcd", "foo", "1")
revision2 := createMockRevisionWithParams("bar-abcd", "bar", "1")
revision3 := createMockRevisionWithParams("foo-wxyz", "foo", "2")
revision4 := createMockRevisionWithParams("bar-wxyz", "bar", "2")
revision1 := createMockRevisionWithParams("foo-abcd", "foo", "1", "100", "")
revision2 := createMockRevisionWithParams("bar-abcd", "bar", "1", "100", "")
revision3 := createMockRevisionWithParams("foo-wxyz", "foo", "2", "50", "tag10")
revision4 := createMockRevisionWithParams("bar-wxyz", "bar", "2", "0", "tag2")
// Validate edge case for catching the sorting issue caused by string comparison
revision5 := createMockRevisionWithParams("foo-wxyz", "foo", "10")
revision6 := createMockRevisionWithParams("bar-wxyz", "bar", "10")
revision5 := createMockRevisionWithParams("foo-wxyz", "foo", "10", "tag1", "tagx")
revision6 := createMockRevisionWithParams("bar-wxyz", "bar", "10", "50", "")

RevisionList := &v1alpha1.RevisionList{Items: []v1alpha1.Revision{
*revision1, *revision2, *revision3, *revision4, *revision5, *revision6}}
Expand All @@ -98,8 +98,8 @@ func TestRevisionListDefaultOutput(t *testing.T) {
}

func TestRevisionListDefaultOutputNoHeaders(t *testing.T) {
revision1 := createMockRevisionWithParams("foo-abcd", "foo", "2")
revision2 := createMockRevisionWithParams("bar-wxyz", "bar", "1")
revision1 := createMockRevisionWithParams("foo-abcd", "foo", "2", "100", "")
revision2 := createMockRevisionWithParams("bar-wxyz", "bar", "1", "100", "")
RevisionList := &v1alpha1.RevisionList{Items: []v1alpha1.Revision{*revision1, *revision2}}
action, output, err := fakeRevisionList([]string{"revision", "list", "--no-headers"}, RevisionList)
assert.NilError(t, err)
Expand All @@ -116,10 +116,10 @@ func TestRevisionListDefaultOutputNoHeaders(t *testing.T) {
}

func TestRevisionListForService(t *testing.T) {
revision1 := createMockRevisionWithParams("foo-abcd", "svc1", "1")
revision2 := createMockRevisionWithParams("bar-wxyz", "svc1", "2")
revision3 := createMockRevisionWithParams("foo-abcd", "svc2", "1")
revision4 := createMockRevisionWithParams("bar-wxyz", "svc2", "2")
revision1 := createMockRevisionWithParams("foo-abcd", "svc1", "1", "50", "")
revision2 := createMockRevisionWithParams("bar-wxyz", "svc1", "2", "50", "")
revision3 := createMockRevisionWithParams("foo-abcd", "svc2", "1", "0", "")
revision4 := createMockRevisionWithParams("bar-wxyz", "svc2", "2", "100", "")
RevisionList := &v1alpha1.RevisionList{Items: []v1alpha1.Revision{*revision1, *revision2, *revision3, *revision4}}
action, output, err := fakeRevisionList([]string{"revision", "list", "-s", "svc1"}, RevisionList)
assert.NilError(t, err)
Expand Down Expand Up @@ -156,7 +156,7 @@ func TestRevisionListForService(t *testing.T) {
}

func TestRevisionListOneOutput(t *testing.T) {
revision := createMockRevisionWithParams("foo-abcd", "foo", "1")
revision := createMockRevisionWithParams("foo-abcd", "foo", "1", "100", "")
RevisionList := &v1alpha1.RevisionList{Items: []v1alpha1.Revision{*revision}}
action, output, err := fakeRevisionList([]string{"revision", "list", "foo-abcd"}, RevisionList)
assert.NilError(t, err)
Expand All @@ -176,7 +176,7 @@ func TestRevisionListOutputWithTwoRevName(t *testing.T) {
assert.ErrorContains(t, err, "'kn revision list' accepts maximum 1 argument")
}

func createMockRevisionWithParams(name, svcName, generation string) *v1alpha1.Revision {
func createMockRevisionWithParams(name, svcName, generation, traffic, tags string) *v1alpha1.Revision {
revision := &v1alpha1.Revision{
TypeMeta: metav1.TypeMeta{
Kind: "Revision",
Expand All @@ -189,6 +189,10 @@ func createMockRevisionWithParams(name, svcName, generation string) *v1alpha1.Re
serving.ServiceLabelKey: svcName,
serving.ConfigurationGenerationLabelKey: generation,
},
Annotations: map[string]string{
"client.knative.dev/traffic": traffic,
"client.knative.dev/tags": tags,
},
},
}
return revision
Expand Down