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

Jump to bottom

Extended support for customizing whereabouts ip-reconciler cron schedule #392

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Extended support for customizing whereabouts ip-reconciler cron schedule #392

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
22 changes: 16 additions & 6 deletions cmd/controlloop/controlloop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"os"
"os/signal"
"strings"
"time"

gocron "github.com/go-co-op/gocron"
Expand All @@ -28,8 +29,9 @@ import (
)

const (
allNamespaces = ""
controllerName = "pod-ip-controlloop"
allNamespaces = ""
controllerName = "pod-ip-controlloop"
reconciler_cron_file = "cron-schedule/reconciler_cron_file"
)

const (
Expand Down Expand Up @@ -66,7 +68,7 @@ func main() {
defer networkController.Shutdown()

s := gocron.NewScheduler(time.UTC)
schedule := cronExpressionFromFlatFile()
schedule := determineCronExpression()
Copy link
Collaborator

Choose a reason for hiding this comment

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

hm, I think you still need to react to updates to the cron expression; meaning, this will only occur when the process starts.

Either you use an higher lib - like fsnotify - or implement something like that using a goroutine that checks if the file's contents have changed, and if so, restart the scheduler.

I guess even exiting the process could be good enough (but worse than reloading the config), since the pod would restart.


_, err = s.Cron(schedule).Do(func() { // user configurable cron expression in install-cni.sh
reconciler.ReconcileIPs(errorChan)
Expand Down Expand Up @@ -164,11 +166,19 @@ func newEventRecorder(broadcaster record.EventBroadcaster) record.EventRecorder
return broadcaster.NewRecorder(scheme.Scheme, corev1.EventSource{Component: controllerName})
}

func cronExpressionFromFlatFile() string {
func determineCronExpression() string {
flatipam, _, err := config.GetFlatIPAM(true, &types.IPAMConfig{}, "")
if err != nil {
_ = logging.Errorf("could not get flatipam: %v", err)
_ = logging.Errorf("could not get flatipam config: %v", err)
os.Exit(couldNotGetFlatIPAM)
}
return flatipam.IPAM.ReconcilerCronExpression

// We read the expression from a file if present, otherwise we use ReconcilerCronExpression
fileContents, err := os.ReadFile("cron-schedule/reconciler_cron_file") // Yuki~ this might be what needs changing if anything. Perhaps it's going to use the wrong filepath.
if err != nil {
_ = logging.Errorf("could not read file: %v, using expression from flatfile: %v", err, flatipam.IPAM.ReconcilerCronExpression)
return flatipam.IPAM.ReconcilerCronExpression
}
logging.Verbosef("using expression: %v", strings.TrimSpace(string(fileContents))) // do i need to trim spaces? idk i think the file would JUST be the expression?
return strings.TrimSpace(string(fileContents))
}
19 changes: 19 additions & 0 deletions doc/crds/daemonset-install.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,19 @@ rules:
- patch
- update
- get

---
apiVersion: v1
kind: ConfigMap
metadata:
name: cron-scheduler-configmap
namespace: kube-system
annotations:
kubernetes.io/description: |
Configmap containing user customizable cronjob schedule
data:
reconciler_cron_file: "30 4 * * *" # Default schedule is once per day at 4:30am. Users may configure this value to their liking.

---
apiVersion: apps/v1
kind: DaemonSet
Expand Down Expand Up @@ -130,10 +143,16 @@ spec:
mountPath: /host/opt/cni/bin
- name: cni-net-dir
mountPath: /host/etc/cni/net.d
- name: cron-scheduler-configmap
mountPath: /cron-schedule
Copy link
Collaborator

Choose a reason for hiding this comment

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

wrong indentation. I guess this is why the daemonset does not start in e2e tests

volumes:
- name: cnibin
hostPath:
path: /opt/cni/bin
- name: cni-net-dir
hostPath:
path: /etc/cni/net.d
- name: cron-scheduler-configmap
configMap:
name: "cron-scheduler-configmap"
defaultMode: 0744
21 changes: 19 additions & 2 deletions doc/extended-configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,16 +134,33 @@ spec:

You'll note that in the `ipam` section there's a lot less parameters than are used in the previous examples.

### Reconciler Cron Expression Configuration (optional)
## Reconciler Cron Expression configuration for clusters via flatfile (optional)

You may want to provide a cron expression to configure how frequently the ip-reconciler runs. This is done via the flatfile.
*NOTE: configuring cron expression prior to cluster launch will only work for **non-Openshift** Kubernetes clusters, such as a vanilla Kubernetes cluster. Skip to the next section if you have an Openshift cluster or a cluster that has already launched.*

Yuki~ do we even need to support configuring via flatfile? Leaving it for now, but hey, food for thought.

You may want to provide a cron expression to configure how frequently the ip-reconciler runs. For clusters that have not yet been launched, this can be configured via the flatfile.

You can speficy the `WHEREABOUTS_RECONCILER_CRON` environment variable in your daemonset definition file to override the default cron expression:
```yaml
env:
- name: WHEREABOUTS_RECONCILER_CRON
value: 30 * * * *
```

## Reconciler Cron Expression Configuration for live clusters via configmap (optional)

Yuki~ README: this section may belong in the CNO since the steps outlined are not technically part of whereabouts. I'm not sure, and suggestions are welcome.
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's remove this line.


You may want to provide a cron expression to configure how frequently the ip-reconciler runs. For **Openshift** Kubernetes clusters, this is done via updating the cron-scheduler-configmap.

You can check that the cron-scheduler-configmap is present by running `oc get configmaps` in the openshift-multus namespace.

To update the cron-scheduler-configmap, run `oc edit configmap cron-scheduler-configmap` and adjust the value to a valid cron expression of your liking. Shortly after, the reconciler schedule will update.

If you are using a non-Openshift cluster, you can do the same steps, but you will need to look for the configmap in the kube-system namespace.

## Installing etcd. (optional)

etcd installation is optional. By default, we recommend the custom resource backend (given in the first example configuration).
Expand Down
4 changes: 2 additions & 2 deletions pkg/types/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
Plugins []*Net `json:"plugins,omitempty"`
}

type RangeConfiguration struct {

Check warning on line 40 in pkg/types/types.go

View workflow job for this annotation

GitHub Actions / test (1.19.x, ubuntu-latest) 

exported type RangeConfiguration should have comment or be unexported
OmitRanges []string `json:"exclude,omitempty"`
Range string `json:"range"`
RangeStart net.IP `json:"range_start,omitempty"`
Expand Down Expand Up @@ -70,10 +70,10 @@
ConfigurationPath string `json:"configuration_path"`
PodName string
PodNamespace string
NetworkName string `json:"network_name,omitempty"`
NetworkName string `json:"network_name,omitempty"`
}

func (ic *IPAMConfig) UnmarshalJSON(data []byte) error {

Check warning on line 76 in pkg/types/types.go

View workflow job for this annotation

GitHub Actions / test (1.19.x, ubuntu-latest) 

exported method IPAMConfig.UnmarshalJSON should have comment or be unexported
type IPAMConfigAlias struct {
Name string
Type string `json:"type"`
Expand Down Expand Up @@ -106,7 +106,7 @@
ConfigurationPath string `json:"configuration_path"`
PodName string
PodNamespace string
NetworkName string `json:"network_name,omitempty"`
NetworkName string `json:"network_name,omitempty"`
}

ipamConfigAlias := IPAMConfigAlias{
Expand Down
2 changes: 1 addition & 1 deletion script/install-cni.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ WHEREABOUTS_RECONCILER_CRON=${WHEREABOUTS_RECONCILER_CRON:-30 4 * * *}

mkdir -p $CNI_CONF_DIR/whereabouts.d
WHEREABOUTS_KUBECONFIG=$CNI_CONF_DIR/whereabouts.d/whereabouts.kubeconfig
WHEREABOUTS_FLATFILE=$CNI_CONF_DIR/whereabouts.d/whereabouts.conf
WHEREABOUTS_FLATFILE=$CNI_CONF_DIR/whereabouts.d/whereabouts.conf # Yuki~ Nikhil's note: imo we should remove "flatfile" from whereabouts vocabulary and call this "WHEREABOUTS_CONF_FILE" instead. Flatfile may be the format but it's confusing naming.
Copy link
Collaborator

Choose a reason for hiding this comment

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

+1 on the naming.

WHEREABOUTS_KUBECONFIG_LITERAL=$(echo "$WHEREABOUTS_KUBECONFIG" | sed -e s'|/host||')

# ------------------------------- Generate a "kube-config"
Expand Down
Loading