Restructure existing code to work with latest version of operator-sdk

[adammw]

WORK IN PROGRESS

Issue #: #522

Description of changes: Restructure existing code to work with latest version of operator-sdk.

Following the recommendation of the operator-sdk migration guide, I created a new operator-sdk project and copied as much of the existing code as possible. Most changes are around the controller logic, as the operator-sdk API has changed from combined Handle() methods which take change events, to Reconcile() which take reconciliation requests of a single object instead.

Open Questions

• Will this code be accepted for a new minor release?
• Should the log code be left alone with the built-in seelog-based utilities or migrated to sigs.k8s.io/controller-runtime/pkg/log

TODO

• Fix/update tests
• Set correct log-levels to reduce log spam
• Increase reconciliation period, 5 seconds seems awfully low

---

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

[mogren]

@adammw Hey, thanks for working on this. We would definitely consider this PR for a new minor release, after enough testing of course. Probably not for v1.6.0, since we are aiming to get that version out pretty soon.

That said, this is a pretty big change. I did take a quick stab at doing the smallest possible change required to update, but that was still a lot of work and didn't really follow the standard pattern used by the operator-framework.

[adammw]

Yeah, I think apart from the logging changes, this is essentially the smallest change as operator sdk API change necessitates splitting up the existing eniconfig controller into two. That being said, the actual value of using controllers in the first place seems to be reasonably low since the ipam module polls the controllers for information periodically, meaning it doesn't directly get notified of changes and they're only applied for new ipam requests.

…

On Mon, Aug 5, 2019, 2:53 PM Claes Mogren ***@***.***> wrote: @adammw <https://github.com/adammw> Hey, thanks for working on this. We would definitely consider this PR for a new minor release, after enough testing of course. Probably not for v1.6.0, since we are aiming to get that version out pretty soon. That said, this is a pretty big change. I did take a quick stab at doing the smallest possible change required to update, but that was still a lot of work and didn't really follow the standard pattern used by the operator-framework. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#560?email_source=notifications&email_token=AABFNAYFC56RUMEWFZVAVCTQDCOMLA5CNFSM4IJAQVA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3TGFXI#issuecomment-518415069>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABFNA2ABSENSKN5EW627SLQDCOMLANCNFSM4IJAQVAQ> .

[adammw]

Another question is how far away the rearchitecture discussed on the roadmap is away, whether this makes sense to merge in a minor version, or wait and skip to the new codebase when it's ready. On Tue, Aug 27, 2019, 5:12 PM Adam Malcontenti-Wilson <adman.com@gmail.com> wrote:

…

Yeah, I think apart from the logging changes, this is essentially the smallest change as operator sdk API change necessitates splitting up the existing eniconfig controller into two. That being said, the actual value of using controllers in the first place seems to be reasonably low since the ipam module polls the controllers for information periodically, meaning it doesn't directly get notified of changes and they're only applied for new ipam requests. On Mon, Aug 5, 2019, 2:53 PM Claes Mogren ***@***.***> wrote: > @adammw <https://github.com/adammw> Hey, thanks for working on this. We > would definitely consider this PR for a new minor release, after enough > testing of course. Probably not for v1.6.0, since we are aiming to get that > version out pretty soon. > > That said, this is a pretty big change. I did take a quick stab at doing > the smallest possible change required to update, but that was still a lot > of work and didn't really follow the standard pattern used by the > operator-framework. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#560?email_source=notifications&email_token=AABFNAYFC56RUMEWFZVAVCTQDCOMLA5CNFSM4IJAQVA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3TGFXI#issuecomment-518415069>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AABFNA2ABSENSKN5EW627SLQDCOMLANCNFSM4IJAQVAQ> > . >

[jaypipes]

@adammw sorry the delay in getting back to you on this! Are you still interested in working on this? There's been quite a few changes since this PR was originally submitted and you'd need to rebase (or likely restart afresh). The 2.x series CNI plugin won't be out until at the earliest next year and we'll still be supporting the 1.x plugin series for years to come, so I think this is still important tech debt to work on.

[mogren]

The last update of this PR was over a year ago, closing it.