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

refactor maestro agent flags. #112

Merged
merged 3 commits into from
Jun 11, 2024
Merged

refactor maestro agent flags. #112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad90aaa
refactor maestro agent flags.
morvencao Jun 6, 2024
b04e66b
change component name.
morvencao Jun 7, 2024
fc8485d
Merge branch 'main' into br_enhance_agent
clyang82 Jun 11, 2024
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
84 changes: 29 additions & 55 deletions cmd/maestro/agent/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,95 +5,69 @@ import (
"flag"
"fmt"
"os"
"os/signal"
"syscall"

"github.com/golang/glog"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
utilflag "k8s.io/component-base/cli/flag"
"k8s.io/component-base/version"
"k8s.io/klog/v2"
ocmfeature "open-cluster-management.io/api/feature"
"open-cluster-management.io/ocm/pkg/common/options"
commonoptions "open-cluster-management.io/ocm/pkg/common/options"
"open-cluster-management.io/ocm/pkg/features"
"open-cluster-management.io/ocm/pkg/work/spoke"
)

var (
commonOptions = options.NewAgentOptions()
commonOptions = commonoptions.NewAgentOptions()
agentOption = spoke.NewWorkloadAgentOptions()
)

// by default uses 1M as the limit for state feedback
var maxJSONRawLength int32 = 1024 * 1024
const maxJSONRawLength int32 = 1024 * 1024

func NewAgentCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "agent",
Short: "Start the maestro agent",
Long: "Start the maestro agent.",
Run: runAgent,
}
agentOption.MaxJSONRawLength = maxJSONRawLength
agentOption.CloudEventsClientCodecs = []string{"manifest", "manifestbundle"}
cfg := spoke.NewWorkAgentConfig(commonOptions, agentOption)
cmdConfig := commonOptions.CommoOpts.
NewControllerCommandConfig("maestro-agent", version.Get(), cfg.RunWorkloadAgent)

cmd := cmdConfig.NewCommandWithContext(context.TODO())
cmd.Use = "agent"
cmd.Short = "Start the Maestro Agent"
cmd.Long = "Start the Maestro Agent"

// check if the flag is already registered to avoid duplicate flag define error
if flag.CommandLine.Lookup("alsologtostderr") != nil {
flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
}

// add klog flags
klog.InitFlags(nil)

fs := cmd.PersistentFlags()
fs.SetNormalizeFunc(utilflag.WordSepNormalizeFunc)
fs.AddGoFlagSet(flag.CommandLine)
commonOptions.CommoOpts.AddFlags(fs)
addFlags(fs)
flags := cmd.Flags()
flags.SetNormalizeFunc(utilflag.WordSepNormalizeFunc)
flags.AddGoFlagSet(flag.CommandLine)

// add common flags
// commonOptions.AddFlags(flags)
// features.SpokeMutableFeatureGate.AddFlag(flags)
// add agent flags
agentOption.AddFlags(flags)
// add alias flags
addFlags(flags)

utilruntime.Must(features.SpokeMutableFeatureGate.Add(ocmfeature.DefaultSpokeWorkFeatureGates))
utilruntime.Must(features.SpokeMutableFeatureGate.Set(fmt.Sprintf("%s=true", ocmfeature.RawFeedbackJsonString)))

return cmd
}

func runAgent(cmd *cobra.Command, args []string) {
ctx, cancel := context.WithCancel(context.Background())

stopCh := make(chan os.Signal, 1)
signal.Notify(stopCh, syscall.SIGINT, syscall.SIGTERM)
go func() {
defer cancel()
<-stopCh
}()

// use mqtt as the default driver
agentOption.MaxJSONRawLength = maxJSONRawLength
cfg := spoke.NewWorkAgentConfig(commonOptions, agentOption)
cmdConfig := commonOptions.CommoOpts.
NewControllerCommandConfig("maestro-agent", version.Get(), cfg.RunWorkloadAgent)
cmdConfig.DisableLeaderElection = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@clyang82 I found this was added in #20, any reason to disable leader election explicitly?

Copy link
Contributor

@skeeey skeeey Jun 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

would you consider to use cmdConfig.NewCommandWithContext(context.TODO()) to build command, e.g. https://github.com/open-cluster-management-io/ocm/blob/main/pkg/cmd/spoke/work.go#L24

so we can get more basic flags from library go

Copy link
Contributor

Choose a reason for hiding this comment

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

no specific reason to disable it at the beginning. just make it works for initial version. We need to enable it based on the current case.

Copy link
Contributor Author

@morvencao morvencao Jun 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Updated this to following https://github.com/open-cluster-management-io/ocm/blob/main/pkg/cmd/spoke/work.go#L24

One question: we have added some flag alias for maestro agent, which are different from work-agent. Eg. --consumer-name instead of --spoke-cluster-name? do we still need these alias flags?

Copy link
Contributor

Choose a reason for hiding this comment

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

what does election leader provide that advisory locks from postgres do not?

locks give us concurrency and parallelism in execution. leader election ties us to a single pod at a time and has proven troublesome with CS in the past.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, i think this is in the context of the agent, which would be in a cluster without the postgres database. nevermind my question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the agent is using in-cluster leader election.


if err := cmdConfig.StartController(ctx); err != nil {
glog.Fatalf("error running command: %v", err)
}
}

// addFlags overrides cluster name and leader leader election flags from the agentOption
func addFlags(fs *pflag.FlagSet) {
// workloadAgentOptions
fs.Int32Var(&maxJSONRawLength, "max-json-raw-length",
maxJSONRawLength, "The maximum size of the JSON raw string returned from status feedback")
fs.DurationVar(&agentOption.StatusSyncInterval, "status-sync-interval",
agentOption.StatusSyncInterval, "Interval to sync resource status to hub")
fs.DurationVar(&agentOption.AppliedManifestWorkEvictionGracePeriod, "resource-eviction-grace-period",
agentOption.AppliedManifestWorkEvictionGracePeriod, "Grace period for resource eviction")
fs.StringVar(&commonOptions.SpokeClusterName, "consumer-name",
commonOptions.SpokeClusterName, "Name of the consumer")
// message broker config file
fs.StringVar(&agentOption.WorkloadSourceConfig, "message-broker-config-file",
agentOption.WorkloadSourceConfig, "The config file path of the message broker, it can be mqtt broker or kafka broker")
fs.StringVar(&agentOption.WorkloadSourceDriver, "message-broker-type", "mqtt", "Message broker type (default: mqtt)")
fs.StringVar(&agentOption.CloudEventsClientID, "agent-client-id",
agentOption.CloudEventsClientID, "The ID of the agent client, by default it is <consumer-id>-work-agent")
fs.StringSliceVar(&agentOption.CloudEventsClientCodecs, "agent-client-codecs",
[]string{"manifest"}, "The codecs of the agent client. The valid codecs are manifest and manifestbundle")

fs.BoolVar(&commonOptions.CommoOpts.CmdConfig.DisableLeaderElection, "disable-leader-election",
true, "Disable leader election.")
Copy link
Contributor

Choose a reason for hiding this comment

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

How long will take to start the agent if enable leader election by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't see difference from the case when leader election is disabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

so can we enable the leader election by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as we discussed, we disable leader election by default.

}
6 changes: 3 additions & 3 deletions templates/agent-template-aro-hcp.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,9 +268,9 @@ objects:
- /usr/local/bin/maestro
- agent
- --consumer-name=$(CONSUMER_NAME)
- --message-broker-type=mqtt
- --message-broker-config-file=/secrets/mqtt/config.yaml
- --agent-client-id=$(CONSUMER_NAME)-work-agent
- --workload-source-driver=mqtt
- --workload-source-config=/secrets/mqtt/config.yaml
- --cloudevents-client-id=$(CONSUMER_NAME)-work-agent
env:
- name: CONSUMER_NAME
valueFrom:
Expand Down
6 changes: 3 additions & 3 deletions templates/agent-template.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,9 +289,9 @@ objects:
- /usr/local/bin/maestro
- agent
- --consumer-name=${CONSUMER_NAME}
- --message-broker-type=mqtt
- --message-broker-config-file=/secrets/mqtt/config.yaml
- --agent-client-id=${CONSUMER_NAME}-work-agent
- --workload-source-driver=mqtt
- --workload-source-config=/secrets/mqtt/config.yaml
- --cloudevents-client-id=${CONSUMER_NAME}-work-agent
volumeMounts:
- name: mqtt
mountPath: /secrets/mqtt
Expand Down
Loading