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

Add affinity and tolerations APIManager configurability for DeploymentConfigs #384

Merged
merged 8 commits into from Jun 23, 2020

Conversation

miguelsorianod
Copy link
Contributor

@miguelsorianod miguelsorianod commented Jun 10, 2020

This PR intends to add configurability of affinity and tolerations in APIManager for DeploymentConfigs (database and non-database ones)

The implementation has the following remarkable points:

  • Reconciliation of 'Affinity' and 'Tolerations' has been implemented for all DeploymentConfigs. This means the reconciliation is executed even if the DeploymentConfig does not have configurability at APIManager level and even if the DeploymentConfig is a database one. It still should work without issues as the default values for those are nil and no changes should be detected. The only new difference in behaviour for database DCs and DCs where APIManager configurability is not provided is that if the user manually changes the values for them directly in the DCs the changes will be reverted by the operator to nil for them.
  • Affinity and Tolerations are deep objects (several levels and multiple fields) and contain arrays (where elements order matters). Reconciliation has been implemented with a DeepEqual comparison and seems to work ok. Changing the order of the specified array elements in APIManager is detected as a change even if the elements are the same, so a reconciliation happens in that case too. This behaviour is no different than changing array elements in a DeploymentConfig in OpenShift.
  • Operator-only functionality

@miguelsorianod
Copy link
Contributor Author

For the moment I've implemented configurability in APIManager level for APIcast production and APIcast staging DeploymentConfigs.

@eguzki please let's first review the implementation to see if we can go forward with this.

Thank you

@eguzki
Copy link
Member

eguzki commented Jun 16, 2020

Looks go.
Go ahead and implement for all 3scale components.

For database DC as well.

@eguzki eguzki assigned miguelsorianod and unassigned eguzki Jun 16, 2020
@miguelsorianod miguelsorianod force-pushed the add-nodeaffinity-and-tolerations branch from de0645c to f89e5e6 Compare June 16, 2020 12:49
@miguelsorianod miguelsorianod changed the title [WIP] Add affinity and tolerations APIManager configurability for non-database deploymentconfigs [WIP] Add affinity and tolerations APIManager configurability for DeploymentConfigs Jun 16, 2020
@miguelsorianod
Copy link
Contributor Author

miguelsorianod commented Jun 16, 2020

I've added fields and implementations for both non-db and db DCs.

As originally commented in issue THREESCALE-5053 there are some elements that do not fit in any specific preexisting section:

system-memcached. I've added two fields in the main 'System' section:
system.memcachedAffinity
system.memcachedTolerations

system-sphinx. I've added a new section called 'sphinx' in 'system'
system.sphinx.affinity
system.sphinx.tolerations

zync-database. I've added two fields in the main 'Zync' section:
zync.databaseAffinity
zync.databaseTolerations

The reasons why sphinx has its own section and the others don't are:

  • I considered the other ones 'databases' and not sphinx. To be honest I think it could be argued whether sphinx is a database or not or whether potentially could be external or not. It's in sort of a middleground between application and a database.
  • I'm thinking on memcached and zync-database potentially being external (zync-database has been requested by some people in the past) so I didn't want to create a new section for fields that might only apply in an 'embedded-only' context. If the PR [WIP] Increase currently supported embedded/external databases granularity #383 related to increased database granularity was performed I'd just have created sections for them too and add them inside an 'embedded' subsection but it's not the case as the changes there have been at least temporarily delayed so I decided to add them directly in the main section and deprecate them in the future if we end up adding granularity for embedded/external DBs.
  • zync-database and system-memcached already have fields in their corresponding main-level component (postgresqlimage field and memcachedImage field)

I also have some doubts regarding the naming of memcached and zync postgresql. If you look at the Jira issue THREESCALE-5053 I already expressed my concern:

memcached: maybe a more generic term name than memcached that is not limited to memcached implementation in case it changes in the future (for example being in redis instead of memcached)? It's true though that we already have memcached-named fields (memcachedImage)
sphinx: maybe a more generic term name than sphinx that is not limited to sphinx implementation in case it changes in the future?
zync database: maybe call it databaseSpec.postgresSQL? maybe postgreSQL? will it potentially change or have possible different dbs? We already have a postgreSQLImage field but might be a good idea to just name it databaseXXX (although we would end up having different namings between the image and the affinity configs then).

Finally, keep in mind that depending on what changes are performed it might additional code migrations (due to possible deprecations of fields, changes in current schema, ...)

I've also just realized that currently some reconcilers would not be reconciling affinity and tolerations. We have to change the mutators/create new mutators for the following items so reconciliation of affinity and nodeaffinity is performed:
· system-memcached
· backend-redis
· system-redis
· system-database (mysql and postgresql)
· system-app
· system-sphinx
· zync-database

The reason for that is that some use the CreateOnlyMutator, others use the Resources mutator only, some others have a custom one, ... so we need to create new custom mutators for each of them.

EDIT: I've also realized that some unit tests are failing due to nil pointer exceptions when dereferencing some intermediate sections of the CR. The issue does not happen when in a real environment and the reason for that is that those intermediate sections are initialized by the controller using the setDefaults method to initialize some CR fields, but because in unit tests the CR's setDefault code is not called the nil pointer happens. How should we proceed? Should I add nil pointer checks in the options providers affinity/tolerations code to check for nil pointers anyway (even if it shouldn't happen in a real scenario)?

Please let's review the current implementation before continuing.

Thank you.

@eguzki
Copy link
Member

eguzki commented Jun 19, 2020

For unittest, you can "initialize" your apimanager objects calling SetDefaults. Then the context will be closer to production code running.

Copy link
Member

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

LGTM so far

@@ -41,19 +41,1353 @@ spec:
type: boolean
productionSpec:
properties:
affinity:
Copy link
Member

Choose a reason for hiding this comment

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

Definitely this CRD file is not human readable anymore. It is already too big.

We have to trust on tooling

@eguzki eguzki removed their assignment Jun 19, 2020
@miguelsorianod miguelsorianod force-pushed the add-nodeaffinity-and-tolerations branch from a0e02a3 to 2eb48ee Compare June 19, 2020 09:52
@miguelsorianod miguelsorianod force-pushed the add-nodeaffinity-and-tolerations branch from 2eb48ee to 5a81cc0 Compare June 23, 2020 09:05
@miguelsorianod miguelsorianod changed the title [WIP] Add affinity and tolerations APIManager configurability for DeploymentConfigs Add affinity and tolerations APIManager configurability for DeploymentConfigs Jun 23, 2020
@miguelsorianod
Copy link
Contributor Author

miguelsorianod commented Jun 23, 2020

New changes made:

  • Updated mutators used for DCs that weren't using the Generic Mutator to reconcile Affinity and Tolerations too
  • Added unit tests
  • Updated documentation

Ready for review @eguzki

@eguzki
Copy link
Member

eguzki commented Jun 23, 2020

Overall LGTM.

I miss some documentation example. Updating API reference is not enough.

Something like, let's configure:

  • node affinity A for apicast, backend listener and backend redis
  • node affinity B for the remaining components

I guess this is going to be useful eventually.

@eguzki eguzki assigned miguelsorianod and unassigned eguzki Jun 23, 2020
@miguelsorianod
Copy link
Contributor Author

Overall LGTM.

I miss some documentation example. Updating API reference is not enough.

Something like, let's configure:

node affinity A for apicast, backend listener and backend redis
node affinity B for the remaining components

I guess this is going to be useful eventually.

Being advanced use cases I considered there was not much point on having specific examples for it but I've added an example anyway 👍

@miguelsorianod miguelsorianod force-pushed the add-nodeaffinity-and-tolerations branch from 3185e63 to 67f1fed Compare June 23, 2020 15:04
@codeclimate
Copy link

codeclimate bot commented Jun 23, 2020

Code Climate has analyzed commit 67f1fed and detected 25 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1
Duplication 20
Style 4

View more on Code Climate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants