Skip to content

Add LastModifiedTime to Status for ScalableTarget and ScalingPolicy resources #40

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
Merged
merged 1 commit into from
Sep 10, 2021
Merged

Add LastModifiedTime to Status for ScalableTarget and ScalingPolicy resources #40

merged 1 commit into from
Sep 10, 2021

Conversation

mbaijal
Copy link
Contributor

@mbaijal mbaijal commented Sep 8, 2021 •
edited
Loading

Description of changes:

Add LastModifiedTime to Status for ScalableTarget and ScalingPolicy resources
TODO: Timezone of CreationTimestamp is different from the CreationTime and LastModifiedTime

Testing:

  • Manually tested that the field is present in the Status of the kubectl get response and gets modified on resource updates.
  • Kind tests pass locally

Output as shown below:

$ kubectl describe scalingpolicy.applicationautoscaling.services.k8s.aws/m-scalingpolicy-may13

Name:         m-scalingpolicy-may13
Namespace:    default
Labels:       <none>
Annotations:  <none>
API Version:  applicationautoscaling.services.k8s.aws/v1alpha1
Kind:         ScalingPolicy
Metadata:
  Creation Timestamp:  2021-09-08T18:35:14Z
  Finalizers:
    finalizers.applicationautoscaling.services.k8s.aws/ScalingPolicy
  Generation:        3
  Resource Version:  2247
  Self Link:         /apis/applicationautoscaling.services.k8s.aws/v1alpha1/namespaces/default/scalingpolicies/m-scalingpolicy-may13
  UID:               <truncated>
Spec:
  Policy Name:         m-scalingpolicy-may13
  Policy Type:         TargetTrackingScaling
  Resource ID:         endpoint/<endpointname>/variant/AllTraffic
  Scalable Dimension:  sagemaker:variant:DesiredInstanceCount
  Service Namespace:   sagemaker
  Target Tracking Scaling Policy Configuration:
    Predefined Metric Specification:
      Predefined Metric Type:  SageMakerVariantInvocationsPerInstance
    Scale In Cooldown:         700
    Scale Out Cooldown:        300
    Target Value:              80
Status:
  Ack Resource Metadata:
    Arn:               arn:aws:autoscaling:<truncated>
    Owner Account ID:  169544399729
  Alarms:
    Alarm ARN:   <truncated>
    Alarm Name:  <truncated>
    Alarm ARN:   <truncated>
    Alarm Name:  <truncated>
  Conditions:
  Creation Time:       2021-09-08T18:35:15Z
  Last Modified Time:  2021-09-08T18:36:31Z

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ack-bot ack-bot requested review from akartsky and jaypipes September 8, 2021 18:43
@mbaijal mbaijal force-pushed the fields-post-release-modifiedtime branch from e28d033 to 2d9432f Compare September 8, 2021 21:58
@mbaijal mbaijal changed the title [merge-after-PR#38] Add LastModifiedTime to Status for ScalableTarget and ScalingPolicy resources Add LastModifiedTime to Status for ScalableTarget and ScalingPolicy resources Sep 8, 2021
mbaijal
mbaijal commented Sep 8, 2021
View reviewed changes
pkg/resource/scalable_target/custom_api.go
svcapitypes "github.com/aws-controllers-k8s/applicationautoscaling-controller/apis/v1alpha1"
svcsdk "github.com/aws/aws-sdk-go/service/applicationautoscaling"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"time"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need reordering

ryansteakley
ryansteakley reviewed Sep 9, 2021
View reviewed changes
pkg/resource/scaling_policy/custom_api.go Outdated

// customSetOutputUpdate sets the LastModifiedTime field to the current time post an update
func (rm *resourceManager) customSetLastModifiedTime(ko *svcapitypes.ScalingPolicy) {
currentTime := metav1.Time{Time: time.Now()}
Copy link
Member

Choose a reason for hiding this comment

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

Will this time.now() local time be the same as the time zone shown in creationTime?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They should all be in UTC now

jaypipes
jaypipes suggested changes Sep 9, 2021
View reviewed changes
Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

Should be UTC since CreationTime is in UTC

@mbaijal mbaijal force-pushed the fields-post-release-modifiedtime branch from 2d9432f to eed1c66 Compare September 10, 2021 21:05
@mbaijal mbaijal force-pushed the fields-post-release-modifiedtime branch from eed1c66 to 1a07061 Compare September 10, 2021 21:07
jaypipes
jaypipes approved these changes Sep 10, 2021
View reviewed changes
Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

👍

@jaypipes
Copy link
Contributor

/lgtm

@ack-bot ack-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 10, 2021
@ack-bot
Copy link
Collaborator

ack-bot commented Sep 10, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jaypipes, mbaijal

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-bot ack-bot merged commit 8a8b825 into aws-controllers-k8s:main Sep 10, 2021
surajkota
surajkota reviewed Sep 10, 2021
View reviewed changes
Copy link
Member

@surajkota surajkota left a comment

Choose a reason for hiding this comment

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

2 things

Do you think it's better to set the modified time = creation time post create instead of current time post api response?

Can you add an assert to integ tests. Notebook lifecycle config has a similar assert

@mbaijal mbaijal mentioned this pull request Sep 13, 2021
Merged
ack-bot pushed a commit that referenced this pull request Sep 13, 2021
@mbaijal
release artifacts for release v0.0.3 (#44)
cdd2eef 
Description of changes:
release artifacts for release v0.0.3

Release Notes draft:
This release includes the following resources and updates:
 - Update ACK Runtime from 0.13.0 to 0.13.2 ( #41 )
    - Please refer to https://github.com/aws-controllers-k8s/runtime/releases for a detailed list of changes
 - The Status field now includes the CreationTime as recorded by the server ( #39 ).
 - The Status field also includes the `LastModifiedTime` which records the time the resource was last updated ( #40 ).
 - Includes updates to the README file ( #37 )


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@mbaijal mbaijal mentioned this pull request Sep 14, 2021
Merged
ack-bot pushed a commit that referenced this pull request Sep 16, 2021
@mbaijal
Use the CreationTime to update LastModifiedTime (#49)
8eb6934 
Issue #, if available:
Fixing [comment](#40 (review)) on previous PR #40 

Description of changes:
 - Use the CreationTime to update LastModifiedTime 
 - Add associated test to check LastModifiedTime

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@surajkota surajkota surajkota left review comments

@ryansteakley ryansteakley ryansteakley left review comments

@akartsky akartsky Awaiting requested review from akartsky

+1 more reviewer

@jaypipes jaypipes jaypipes approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jaypipes jaypipes

Labels

approved lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mbaijal @jaypipes @ack-bot @surajkota @ryansteakley