keikoproj / instance-manager

Create and manage instance groups with Kubernetes
Apache License 2.0
155 stars 40 forks source link

Required selectors in configs #329

Open kevdowney opened 3 years ago

kevdowney commented 3 years ago

Is this a BUG REPORT or FEATURE REQUEST?: FEATURE

What happened: We would like to treat certain conditionals as a specific rules match when the annotations are required.

Currently the conditionals are pure overrides and seem to work as a cascading config override so the last matching selector config wins but having a selector can be required in certain instances like architecture.

Currently this is how we have to specify architecture support but this does not enforce required selector to match.

 conditionals: | 
   - annotationSelector: 'instancemgr.keikoproj.io/arch = arm64,instancemgr.keikoproj.io/os-family = amazonlinux2' 
     defaults: 
      spec: 
        eks: 
          configuration: 
            image: {{ .ArmImage }} 

What you expected to happen: This could be done via a required annotation required.instancemgr.keikoproj.io/arch = arm64 or required conditionals section so any rules miss would result in an error condition.

 required-conditionals: | 
   - annotationSelector: 'instancemgr.keikoproj.io/arch = arm64,instancemgr.keikoproj.io/os-family = amazonlinux2' 
     defaults: 
      spec: 
        eks: 
          configuration: 
            image: {{ .ArmImage }} 

How to reproduce it (as minimally and precisely as possible):

Anything else we need to know?:

Environment:

kubectl version -o yaml

Other debugging information (if applicable):

kubectl describe instancegroup <ig-name>
kubectl logs <instance-manager pod>
eytan-avisror commented 3 years ago

Maybe a cleaner API would be

conditionals: | 
   - annotationSelector: 'instancemgr.keikoproj.io/arch = arm64,instancemgr.keikoproj.io/os-family = amazonlinux2' 
>    required: true
     defaults: 
      spec: 
        eks: 
          configuration: 
            image: {{ .ArmImage }} 

But if I understand the ask correctly, it would mean if it's REQUIRED and the annotations are matching the selector, then we DO NOT allow the global default to be used as fallback.

In the case of image this would mean a failure, but it might not mean a failure in cases of other specs such as ones that have default values or ones that are not required fields.

@backjo any thoughts on this?

backjo commented 3 years ago

I'm not sure I fully understand the use case - if I have it right, the intention is to enforce the existence of annotations on IGs dynamically through a rule mechanism - is that correct?

kevdowney commented 3 years ago

This use-case can be discretely categorized as there’s specific required annotations that would be important for certain IGs, architecture is one good example you cannot fallback to an x86 configuration Ami when you need that IG to be exclusively arm64

Get Outlook for iOShttps://aka.ms/o0ukef


From: Jonah Back @.> Sent: Thursday, September 9, 2021 5:22:06 PM To: keikoproj/instance-manager @.> Cc: Kevin Downey @.>; Author @.> Subject: Re: [keikoproj/instance-manager] Required selectors in configs (#329)

I'm not sure I fully understand the use case - if I have it right, the intention is to enforce the existence of annotations on IGs dynamically through a rule mechanism - is that correct?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/keikoproj/instance-manager/issues/329#issuecomment-916531089, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAIQJMAJXAPS426X3LAZSDDUBFFS5ANCNFSM5DTHAUBQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

eytan-avisror commented 3 years ago

@kevdowney I think this is more of a client problem. What you want is a condition where if {{ .ArmImage }} is missing / blank, there is no fallback on global defaults (fail instead, due to missing AMI).

I think this is the case currently, meaning if you try to use the below, an annotated IG will end up in failed state:

conditionals: | 
   - annotationSelector: 'instancemgr.keikoproj.io/arch = arm64,instancemgr.keikoproj.io/os-family = amazonlinux2' 
     defaults: 
      spec: 
        eks: 
          configuration: 
            image: ""

But I think the problem is if you REMOVE the conditional entirely, then you end up with a situation that annotated IGs gets the global default - which could be an incompatible AMI.

TBH, not sure we can/should support this use case, or I don't see how it's possible. If the conditional is gone, there is no correlation to the annotations any longer, how can the controller know they mean anything?

The only possible change I see, is to add some sort of controller logic for the annotation instancemgr.keikoproj.io/arch = arm64 and possibly try to check if the image provided is in the correct architecture (if the API exposes this).

Otherwise I don't see how we can achieve what you're asking.