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

Supports for alarm notification server: #102

Merged

Conversation

Jennifer-chen-rh
Copy link
Collaborator

@Jennifer-chen-rh Jennifer-chen-rh commented Jun 10, 2024

Create the alarm notification server.
Manage in memory alarm subscriptions base on persist storage (populated by alarm subscription server).
Prebuilt subscription filter terms + limited optimization
Accept the alarms, match the filters and build and send out notification packet based on matched subscription.

This PR supports basic alert packet flow. In future the filter match will have more optimization and the alert to notification the attributes may need fine tuned.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 10, 2024
@openshift-ci openshift-ci bot requested review from danielerez and irinamihai June 10, 2024 20:01
@Jennifer-chen-rh Jennifer-chen-rh force-pushed the notification_server branch 3 times, most recently from c70e00d to d4fb607 Compare June 11, 2024 20:39
@Jennifer-chen-rh Jennifer-chen-rh changed the title WIP: Supports for alarm notification server: Supports for alarm notification server: Jun 11, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 11, 2024
@Jennifer-chen-rh Jennifer-chen-rh marked this pull request as draft June 11, 2024 20:49
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 11, 2024
@Jennifer-chen-rh Jennifer-chen-rh marked this pull request as ready for review June 11, 2024 20:51
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 11, 2024
@openshift-ci openshift-ci bot requested a review from jhernand June 11, 2024 20:51
Copy link
Collaborator

@danielerez danielerez left a comment

Choose a reason for hiding this comment

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

Partially reviewed

README.md Outdated
#### Alarm Notification server

The alarm-notification-server should use together with alarm subscription server. The alarm subscription severs accept and manage the alarm subscriptions. The alarm notificaton servers synch the alarm subscriptions via perisist storage. To use the configmap to persist the subscriptions, the namespace "orantest" should be created at hub cluster for now. The alarm notification server accept the alerts, match the subscription filter, build and send out the alarm notification based on url in the subscription.
Copy link
Collaborator

Choose a reason for hiding this comment

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

typos:

  • severs -> server
  • servers -> server (i.e. singular, for consistency)

nit:
Maybe worth adding a note that the "orantest" namespace is a temporary solution (IIUC).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks for the comments. Changed. Please see updated version.

continue
}

/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be clearer to add a TODO note here (i.e. so it won't look like a leftover)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cleaned up.

jq.Any("$alarmEvent", request.Object),
)

//stream := data.Pour(obj)
Copy link
Collaborator

Choose a reason for hiding this comment

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

needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cleaned up


//stream := data.Pour(obj)

go func(pkt data.Object) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a matter of style, but consider avoiding an animus function if not mandatory.
Or, add a comment before the func explaining what it does.
Just to make it a bit clearer:)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added the comment before the function. Keep like this so can easily to view the context of what passed in the routine.

return
}

/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

needed for later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cleaned up.

b.selectorParser = selectorParser
}

/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

still needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cleaned up.

// use 2 sec first
httpClient := http.Client{Timeout: 2 * time.Second}

if singleAlarmNotificationHandle != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be confusing if the var is simply called 'alarmNotificationHandler'?

Copy link
Collaborator Author

@Jennifer-chen-rh Jennifer-chen-rh Jun 14, 2024

Choose a reason for hiding this comment

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

This is good question. I added 'single' here to label the singleton of the alarmNotificationHandler object. Let's wait for 3rd people to break the even. I do not mind to change it if more people think 'alarmNotificationHandler' is more readable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I see. It's not crucial, I don't have a strong opinion about it actually. It can stay as is.

Copy link
Collaborator

@danielerez danielerez left a comment

Choose a reason for hiding this comment

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

Looks good. Mostly minor comments.

// use 2 sec first
httpClient := http.Client{Timeout: 2 * time.Second}

if singleAlarmNotificationHandle != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, I see. It's not crucial, I don't have a strong opinion about it actually. It can stay as is.


// create persist storeage option
persistStore := persiststorage.NewKubeConfigMapStore().
SetNameSpace(TestNamespace).
Copy link
Collaborator

Choose a reason for hiding this comment

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

The convention is usually one word for namespace, I.e. SetNamespace

// create persist storeage option
persistStore := persiststorage.NewKubeConfigMapStore().
SetNameSpace(TestNamespace).
SetName(TestConfigmapName).
Copy link
Collaborator

Choose a reason for hiding this comment

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

TestConfigmapName is a constant just for current testings, right? It probably should be customizable later on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is temperate for testConfigurationName. It will have a official name. Let me update this and propose a potential official name. Please see next commit/update.

watcher, err := s.client.Watch(ctx, &corev1.ConfigMapList{}, &opt)

go func() {
for {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need some tests for this logic?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You mean test.go? The subscription server will write the configmap. Alarm server will update the new map in the memory. I did test manually with 2 server instances (1 subscription, 1 notification) by adding/deleting the subscriptions. In the test.go, can we support 2 handler instances and mimic the configmap change? Please let me know.


subIdSet := h.getSubscriptionIdsFromAlarm(ctx, request.Object)

//now look id_set and send http packets to URIs
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo? e.g. "loop over IDs"

}

var obj data.Object
// alarmNotificationType needs to be added
Copy link
Collaborator

Choose a reason for hiding this comment

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

worth adding 'TODO: '

(*b.subscriptionInfoMap)[subId] = subInfo

//for now use path 0 only
//to be fixed with full path for quicker search
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider adding 'TODO: '

var uris string
err = jqTool.Evaluate(`.callback`, value, &uris)
if err != nil {
panic("the callback is mandary for subscription")
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/mandary/mandatory


func (h *alarmNotificationHandler) getSubscriptionIdsFromAlarm(ctx context.Context, alarm data.Object) (result alarmSubIdSet) {

h.subscriptionMapMemoryLock.RLock()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need the lock here actually? Maybe add some comment for explanation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I will add the comments.


// AlarmNotificationManagerHandlerBuilder contains the data and logic needed to create a new alarm notification
// collection handler. Don't create instances of this type directly, use the
// NewAlarmNotificationHandler function instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment isn't accurate, probably the result of copy & paste?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes copy paste. Will update.

// AlarmNotificationManagerHandlerBuilder contains the data and logic needed to create a new alarm notification
// collection handler. Don't create instances of this type directly, use the
// NewAlarmNotificationHandler function instead.
type alarmNotificationHandlerBuilder struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This builder and the actual object should be public (start with uppercase) otherwise there may be weird situations if other parts of the code want to explicitly declare variables. For example, this would be impossible:

// We have the "err" already declared in an outer scope, and
// we don't want to declare a new one here.
var handler service.alarmNotificationHandler
handler, err = service.NewAlarmNotificationHandler().Build()  // <-- This will not compile because the type isn't public

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually it built. But will make the change

type alarmNotificationHandlerBuilder struct {
logger *slog.Logger
loggingWrapper func(http.RoundTripper) http.RoundTripper
cloudID string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the cloudID actually needed by this handler? If not then we should remove it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The alarm notification server will not communicate with the clients directly but it is a part/component of our oran api. Why do not need this cloudID identifier for this server?


// alarmNotificationHander knows how to respond to requests to list deployment managers.
// Don't create instances of this type directly, use the NewAlarmNotificationHandler function
// instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment isn't accurate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will update

logger *slog.Logger
loggingWrapper func(http.RoundTripper) http.RoundTripper
cloudID string
extensions []string
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe extensions aren't needed here either. Can you check and remove them if they aren't used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes will remove.

}

// NewAlarmNotificationHandler creates a builder that can then be used to configure and create a
// handler for the collection of deployment managers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment isn't accurate.

extensions []string
jsonAPI jsoniter.API
selectorEvaluator *search.SelectorEvaluator
jqTool *jq.Tool
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need a jq tool here? Please check and remove if not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we use this and will keep it.


if singleAlarmNotificationHandle != nil {
panic("There is an existing handler instance")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should not do this check: it is perfectly OK to create multiple alarm notification handlers. Maybe one will receive notifications from Prometheus alert manager and another will receive them from something else, a hardware manager, for example.

If we really needed to ensure that there is only one instance it shouldn't be at this level, but at the level of the code that creates the handler. And to do it correctly we would need to be careful with how we do the check: using a nil check like here doesn't work well when there are multiple goroutines working in parallel.

But lets forget about that: just remove the check, I'd say.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, and we shouldn't be using "panic" anyhow, better return an error.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made it singleton for the configmap update call back. It is also workable to make global array/arrays. Not big deal.

For now I will change not panic but log an error. (change to handle multiple instances only when needed)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed

"CloudID", b.cloudID,
)

err = result.recoveryFromPersistStore(ctx)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are doing additional work after creating the object, and that work may fail, like here, then it is better to use a temporary variable to make sure that we will never return a result that is half baked. For example:

// Create and populate the object:
handler := &alarmNotificationHandler{...}

// Do other things with the "handler" variable, which mail fail.

// When all preparations succeeded copy the temporary variable
// to the result:
result = handler
return

if err != nil {
b.logger.Error(
"alarmNotificationHandler failed to recovery from persistStore ",
slog.String(": ", err.Error()),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will return in JSON log message like this:

{
  ": ": "the details of the error"
}

That looks weird. Can we use slog.String("error", err.Error()) instead?

err = persiststorage.ProcessChangesWithFunction(h.persistStore, ctx, ProcessStorageChanges)

if err != nil {
panic("failed to launch watcher")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Try to avoid panics, return the error instead.

}

func (h *alarmNotificationHandler) watchPersistStore(ctx context.Context) (err error) {
err = persiststorage.ProcessChangesWithFunction(h.persistStore, ctx, ProcessStorageChanges)
Copy link
Collaborator

Choose a reason for hiding this comment

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

ProcessStorageChanges should be a private method of the handler, something like this:

func (h *alarmNotificationHandler) processStorageChanges(...) {
        ....
}

Then here you can pass that method as parameter:

err = persiststorage.ProcessChangesWithFunction(h.persistStore, ctx, h.processStorageChanges)

"github.com/openshift-kni/oran-o2ims/internal/k8s"
)

var _ = Describe("alarm Notification handler", func() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great to have tests from the beginning, thanks @Jennifer-chen-rh!


type subscriptionInfo struct {
filters search.Selector
uris string
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this uris is the callback URL, is it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes

}

router.HandleFunc(
"/o2ims-infrastructureMonitoring/{version}/alarmNotifications",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This URL path makes this look like this is a standard O-RAN O2 IMS endpoint, but it isn't, it is just the place where we receive notifications from alert manager, which isn't defined in O-RAN. I think it should be just /.

@@ -249,3 +253,45 @@ func (s *KubeConfigMapStore) ProcessChanges(ctx context.Context, dataMap **map[s

return
}

func (s *KubeConfigMapStore) ProcessChangesWithFunction(ctx context.Context, function ProcessFunc) (err error) {
raw_opt := metav1.SingleObject(metav1.ObjectMeta{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Try to use camelCase for variables: rawOpt.

newMap := map[string]data.Object{}
for k, value := range configmap.Data {
var object data.Object
err = (*s.jsonAPI).Unmarshal([]byte(value), &object)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the explicit pointer dereference? Should work without it: s.jsonAPI.Unmarshal(...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

make: *** [Makefile:292: binary] Error 1```
The persistent storage keep a pointer to the same jsonAPI object used by its owner handler

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I understand now. s.jsonAPI should not be a pointer because the target is an interface and in Go variables of interface types are already pointers. So you should to change the jsonAPI field of the KubeConfigMapStore struct from *jsoniter.API to just jsoniter.API. Then here you can use directly s.jsonAPI.Unmarshal(...).

Copy link
Collaborator Author

@Jennifer-chen-rh Jennifer-chen-rh Jul 2, 2024

Choose a reason for hiding this comment

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

If we use the pointer, we can avoid to create the new and identical object created by the owner handler. If you do not have strong opinion, I would prefer to use the pointer.

Copy link
Collaborator

@jhernand jhernand Jul 3, 2024

Choose a reason for hiding this comment

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

I do have a strong opinion . Not that it introduces a bug, but it is really uncommon and unnecessary to use a pointer to an interface in general, and in this case in particular.

When you declare a variable like var jsonAPI jsoniter.API what the Go compiler will do is create a structure with two pointers: one pointer to the table describing the methods of the interface (know as an itable) and another pointer to the object itself. When you then do something like jsonAPI = x what you are doing is copying those two pointers, not creating a new object.

If you do var jsonAPI *jsoniter.API and then jsonAPI = &x what you are doing is copying the address of the x variable. That isn't necessary in general, and in particular not necessary in this case. I addition it means that the x object can't be stored in the stack because the Go compiler can't know if it will be used after the current scope is closed: that is known in Go as "escaping" to the heap. Finally it means that you can do *jsonAPI = &y and the result will be that the code that uses x would now surprisingly be using y instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jhernand Thanks for long explanation. Really appreciate it! Will change it later today.

@@ -225,8 +224,19 @@ func (r *AlarmFetcher) FetchItems(
return
}

alarmMapper, err := NewAlarmMapper().
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is great!

Copy link
Collaborator

Choose a reason for hiding this comment

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

And, can we create it once and reuse it?

}

// AlarmMapper creates a builder that can then be used to configure
// and create a handler for the AlarmMapper.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Review the comments: this isn't accurate, it talks about a "handler" but there is no handler here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right. Can be something like: NewAlarmMapper creates a builder that can be used to create a mapper of Alertmanager alerts to AlarmEventRecord objects.

if b.backendClient == nil {
err = errors.New("backend client is mandatory")
return
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about the resource server URL and token? Aren't those required?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@danielerez can you please answer Juan's question?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So a validation for the URL would is indeed needed (as it's mandatory). But the token is optional, as it fallbacks to the backend token if missing. See:

}

// Map an Alert to an O2 Alarm object.
func (r *AlarmMapper) MapAlertItem(ctx context.Context,
Copy link
Collaborator

Choose a reason for hiding this comment

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

As this is an alert mapper there is no need to repeat it in the name of the method. I'd suggest just Map. Alternatively you could just return the function from the builder, and then there is no need for another name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point.

@@ -15,8 +15,8 @@ import (
)

type KubeConfigMapStore struct {
Name string
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it wasn't introduced in this patch, but I just noticed that this type isn't using the builder pattern like in all other types of the project. Please take note and fix it (in a different pull request).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK. Will do that.

if b.backendClient == nil {
err = errors.New("backend client is mandatory")
return
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So a validation for the URL would is indeed needed (as it's mandatory). But the token is optional, as it fallbacks to the backend token if missing. See:

type AlarmMapperBuilder struct {
logger *slog.Logger
backendClient *http.Client
//transportWrapper func(http.RoundTripper) http.RoundTripper
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can be removed.

}

// AlarmMapper creates a builder that can then be used to configure
// and create a handler for the AlarmMapper.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Right. Can be something like: NewAlarmMapper creates a builder that can be used to create a mapper of Alertmanager alerts to AlarmEventRecord objects.

@Jennifer-chen-rh Jennifer-chen-rh force-pushed the notification_server branch 4 times, most recently from bd45555 to 3f01b08 Compare July 3, 2024 21:08
@jhernand
Copy link
Collaborator

jhernand commented Jul 4, 2024

/approve
/lgtm

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 4, 2024
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2024
   - Create the alarm notification handler and alarm notification server.
   - Add user passed in optional namespace and configmap_name for subscription server and notification server
   - Manage alarm subscriptions in memory base on persist storage (configuration map populated by alarm subscription server).
   - Prebuilt subscription filter terms
   - Accept the alarts from alertManager, map the received alert to alarm, match the filters and build and send out notification packet based on matched subscription
   - Daniel E authored the alert to alarm mapping portion in this commit, and the same code used by alarm server.
   - Enhanced subscription handler
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2024
@jhernand
Copy link
Collaborator

jhernand commented Jul 8, 2024

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2024
Copy link

openshift-ci bot commented Jul 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhernand

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit f48efaf into openshift-kni:main Jul 8, 2024
8 checks passed
@Jennifer-chen-rh Jennifer-chen-rh deleted the notification_server branch July 19, 2024 13:05
openshift-merge-bot bot pushed a commit that referenced this pull request Aug 7, 2024
* MGMT-18232: Avoid `controller-gen` crash (#108)

Version of `controller-gen` 0.13.0 crashes like this when we execute
`make install`:

```
/home/user/repos/oran-o2ims/bin/controller-gen rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xa0de0f]goroutine 1 [running]:
go/types.(*Checker).handleBailout(0xc0004aa600, 0xc000e42a18)
        /usr/lib/golang/src/go/types/check.go:367 +0x88
panic({0xbc7860?, 0x12b48a0?})
        /usr/lib/golang/src/runtime/panic.go:770 +0x132
go/types.(*StdSizes).Sizeof(0x0, {0xdc35b8, 0x12bd080})
        /usr/lib/golang/src/go/types/sizes.go:228 +0x30f
go/types.(*Config).sizeof(...)
        /usr/lib/golang/src/go/types/sizes.go:333
go/types.representableConst.func1({0xdc35b8?, 0x12bd080?})
        /usr/lib/golang/src/go/types/const.go:76 +0x9e
go/types.representableConst({0xdc9990, 0x1287f80}, 0xc0004aa600, 0x12bd080, 0xc000e42188)
        /usr/lib/golang/src/go/types/const.go:92 +0x192
go/types.(*Checker).representation(0xc0004aa600, 0xc000f397c0, 0x12bd080)
        /usr/lib/golang/src/go/types/const.go:256 +0x65
go/types.(*Checker).implicitTypeAndValue(0xc0004aa600, 0xc000f397c0, {0xdc35e0, 0xc0000b2700})
        /usr/lib/golang/src/go/types/expr.go:375 +0x2d7
go/types.(*Checker).assignment(0xc0004aa600, 0xc000f397c0, {0xdc35e0, 0xc0000b2700}, {0xc9528c, 0x14})
        /usr/lib/golang/src/go/types/assignments.go:52 +0x2e5
go/types.(*Checker).initConst(0xc0004aa600, 0xc00138f1a0, 0xc000f397c0)
        /usr/lib/golang/src/go/types/assignments.go:126 +0x2c5
go/types.(*Checker).constDecl(0xc0004aa600, 0xc00138f1a0, {0xdc6200, 0xc0006f9dc0}, {0xdc6200, 0xc0006f9de0}, 0x0)
        /usr/lib/golang/src/go/types/decl.go:490 +0x311
go/types.(*Checker).objDecl(0xc0004aa600, {0xdcf120, 0xc00138f1a0}, 0x0)
        /usr/lib/golang/src/go/types/decl.go:191 +0xa49
go/types.(*Checker).packageObjects(0xc0004aa600)
        /usr/lib/golang/src/go/types/resolver.go:693 +0x4dd
go/types.(*Checker).checkFiles(0xc0004aa600, {0xc00027e870, 0x2, 0x2})
        /usr/lib/golang/src/go/types/check.go:408 +0x1a5
go/types.(*Checker).Files(...)
        /usr/lib/golang/src/go/types/check.go:372
sigs.k8s.io/controller-tools/pkg/loader.(*loader).typeCheck(0xc00023f440, 0xc000992e00)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/loader.go:286 +0x36a
sigs.k8s.io/controller-tools/pkg/loader.(*Package).NeedTypesInfo(0xc000992e00)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/loader.go:99 +0x39
sigs.k8s.io/controller-tools/pkg/loader.(*TypeChecker).check(0xc000f3b560, 0xc000992e00)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/refs.go:268 +0x2b7
sigs.k8s.io/controller-tools/pkg/loader.(*TypeChecker).Check(...)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/refs.go:216
sigs.k8s.io/controller-tools/pkg/crd.(*Parser).AddPackage(0xc00108d2c0, 0xc000992e00)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/crd/parser.go:224 +0xfd
sigs.k8s.io/controller-tools/pkg/crd.(*Parser).NeedPackage(0xc00108d2c0, 0xc000992e00)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/crd/parser.go:242 +0xee
sigs.k8s.io/controller-tools/pkg/crd.Generator.Generate({0x0, 0x0, 0x0, {0x0, 0x0, 0x0}, 0x0, {0x0, 0x0}, {0x0, ...}}, ...)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/crd/gen.go:116 +0x128
sigs.k8s.io/controller-tools/pkg/genall.(*Runtime).Run(0xc000468ab0)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/genall/genall.go:273 +0x23d
main.main.func1(0xc00050c300?, {0xc0002346e0?, 0x4?, 0xc8af70?})
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/cmd/controller-gen/main.go:176 +0x6a
github.com/spf13/cobra.(*Command).execute(0xc000202c08, {0xc0000361f0, 0x5, 0x5})
        /home/user/go/pkg/mod/github.com/spf13/[email protected]/command.go:940 +0x882
github.com/spf13/cobra.(*Command).ExecuteC(0xc000202c08)
        /home/user/go/pkg/mod/github.com/spf13/[email protected]/command.go:1068 +0x3a5
github.com/spf13/cobra.(*Command).Execute(...)
        /home/user/go/pkg/mod/github.com/spf13/[email protected]/command.go:992
main.main()
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/cmd/controller-gen/main.go:200 +0x2f6
make: *** [Makefile:113: manifests] Error 2
```

I didn't find a bug report for this, but the latest version (0.15.0)
works correctly. This patch updates the project to use that version.
Note that that has some minor effects in the generated custom resource
definitions: the newer version uses `description: |-` instead of just
`description: ...`. That is also included in the patch.

Related: https://issues.redhat.com/browse/MGMT-18232

Signed-off-by: Juan Hernandez <[email protected]>

* NO-ISSUE: Use Go 1.21 in all modules (#110)

The recently added `api/hardwaremanagement` module uses Go 1.22. That
wasn't intentional. This patch changes it to use Go 1.21 and the same
dependencies than the rest of the project.

Related: #109

Signed-off-by: Juan Hernandez <[email protected]>

* NO-ISSUE: Move selector logic from handlers to adapter (#117)

Currently we apply the selector (the `filter` query parameter) in all
the handlers. This patch moves the logic to the adapter, which is where
it belongs.

Signed-off-by: Juan Hernandez <[email protected]>

* NO-ISSUE: remove redundant jsonAPI from handlers (#119)

Removed unsed `jsonAPI` from handlers (can be added ad-hoc when needed).

* NO-ISSUE: fix typos related to alarms (#112)

* Supports for alarm notification server: (#102)

- Create the alarm notification handler and alarm notification server.
   - Add user passed in optional namespace and configmap_name for subscription server and notification server
   - Manage alarm subscriptions in memory base on persist storage (configuration map populated by alarm subscription server).
   - Prebuilt subscription filter terms
   - Accept the alarts from alertManager, map the received alert to alarm, match the filters and build and send out notification packet based on matched subscription
   - Daniel E authored the alert to alarm mapping portion in this commit, and the same code used by alarm server.
   - Enhanced subscription handler

* CNF-13590: Use the vendor directory when building images (#128)

Description:
- remove the vendor directory from .gitignore and .containerignore
  so that it gets copied into the image with the rest of the
  content
- add -mod=vendor to go build commands

* Add repo owners

* Add .gitattributes (#130)

---------

Signed-off-by: Juan Hernandez <[email protected]>
Co-authored-by: Juan Hernández <[email protected]>
Co-authored-by: Daniel Erez <[email protected]>
Co-authored-by: Jennifer-chen-rh <[email protected]>
Co-authored-by: Irina Mihai <[email protected]>
tliu2021 pushed a commit to tliu2021/oran-o2ims that referenced this pull request Aug 12, 2024
* MGMT-18232: Avoid `controller-gen` crash (openshift-kni#108)

Version of `controller-gen` 0.13.0 crashes like this when we execute
`make install`:

```
/home/user/repos/oran-o2ims/bin/controller-gen rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xa0de0f]goroutine 1 [running]:
go/types.(*Checker).handleBailout(0xc0004aa600, 0xc000e42a18)
        /usr/lib/golang/src/go/types/check.go:367 +0x88
panic({0xbc7860?, 0x12b48a0?})
        /usr/lib/golang/src/runtime/panic.go:770 +0x132
go/types.(*StdSizes).Sizeof(0x0, {0xdc35b8, 0x12bd080})
        /usr/lib/golang/src/go/types/sizes.go:228 +0x30f
go/types.(*Config).sizeof(...)
        /usr/lib/golang/src/go/types/sizes.go:333
go/types.representableConst.func1({0xdc35b8?, 0x12bd080?})
        /usr/lib/golang/src/go/types/const.go:76 +0x9e
go/types.representableConst({0xdc9990, 0x1287f80}, 0xc0004aa600, 0x12bd080, 0xc000e42188)
        /usr/lib/golang/src/go/types/const.go:92 +0x192
go/types.(*Checker).representation(0xc0004aa600, 0xc000f397c0, 0x12bd080)
        /usr/lib/golang/src/go/types/const.go:256 +0x65
go/types.(*Checker).implicitTypeAndValue(0xc0004aa600, 0xc000f397c0, {0xdc35e0, 0xc0000b2700})
        /usr/lib/golang/src/go/types/expr.go:375 +0x2d7
go/types.(*Checker).assignment(0xc0004aa600, 0xc000f397c0, {0xdc35e0, 0xc0000b2700}, {0xc9528c, 0x14})
        /usr/lib/golang/src/go/types/assignments.go:52 +0x2e5
go/types.(*Checker).initConst(0xc0004aa600, 0xc00138f1a0, 0xc000f397c0)
        /usr/lib/golang/src/go/types/assignments.go:126 +0x2c5
go/types.(*Checker).constDecl(0xc0004aa600, 0xc00138f1a0, {0xdc6200, 0xc0006f9dc0}, {0xdc6200, 0xc0006f9de0}, 0x0)
        /usr/lib/golang/src/go/types/decl.go:490 +0x311
go/types.(*Checker).objDecl(0xc0004aa600, {0xdcf120, 0xc00138f1a0}, 0x0)
        /usr/lib/golang/src/go/types/decl.go:191 +0xa49
go/types.(*Checker).packageObjects(0xc0004aa600)
        /usr/lib/golang/src/go/types/resolver.go:693 +0x4dd
go/types.(*Checker).checkFiles(0xc0004aa600, {0xc00027e870, 0x2, 0x2})
        /usr/lib/golang/src/go/types/check.go:408 +0x1a5
go/types.(*Checker).Files(...)
        /usr/lib/golang/src/go/types/check.go:372
sigs.k8s.io/controller-tools/pkg/loader.(*loader).typeCheck(0xc00023f440, 0xc000992e00)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/loader.go:286 +0x36a
sigs.k8s.io/controller-tools/pkg/loader.(*Package).NeedTypesInfo(0xc000992e00)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/loader.go:99 +0x39
sigs.k8s.io/controller-tools/pkg/loader.(*TypeChecker).check(0xc000f3b560, 0xc000992e00)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/refs.go:268 +0x2b7
sigs.k8s.io/controller-tools/pkg/loader.(*TypeChecker).Check(...)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/refs.go:216
sigs.k8s.io/controller-tools/pkg/crd.(*Parser).AddPackage(0xc00108d2c0, 0xc000992e00)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/crd/parser.go:224 +0xfd
sigs.k8s.io/controller-tools/pkg/crd.(*Parser).NeedPackage(0xc00108d2c0, 0xc000992e00)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/crd/parser.go:242 +0xee
sigs.k8s.io/controller-tools/pkg/crd.Generator.Generate({0x0, 0x0, 0x0, {0x0, 0x0, 0x0}, 0x0, {0x0, 0x0}, {0x0, ...}}, ...)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/crd/gen.go:116 +0x128
sigs.k8s.io/controller-tools/pkg/genall.(*Runtime).Run(0xc000468ab0)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/genall/genall.go:273 +0x23d
main.main.func1(0xc00050c300?, {0xc0002346e0?, 0x4?, 0xc8af70?})
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/cmd/controller-gen/main.go:176 +0x6a
github.com/spf13/cobra.(*Command).execute(0xc000202c08, {0xc0000361f0, 0x5, 0x5})
        /home/user/go/pkg/mod/github.com/spf13/[email protected]/command.go:940 +0x882
github.com/spf13/cobra.(*Command).ExecuteC(0xc000202c08)
        /home/user/go/pkg/mod/github.com/spf13/[email protected]/command.go:1068 +0x3a5
github.com/spf13/cobra.(*Command).Execute(...)
        /home/user/go/pkg/mod/github.com/spf13/[email protected]/command.go:992
main.main()
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/cmd/controller-gen/main.go:200 +0x2f6
make: *** [Makefile:113: manifests] Error 2
```

I didn't find a bug report for this, but the latest version (0.15.0)
works correctly. This patch updates the project to use that version.
Note that that has some minor effects in the generated custom resource
definitions: the newer version uses `description: |-` instead of just
`description: ...`. That is also included in the patch.

Related: https://issues.redhat.com/browse/MGMT-18232

Signed-off-by: Juan Hernandez <[email protected]>

* NO-ISSUE: Use Go 1.21 in all modules (openshift-kni#110)

The recently added `api/hardwaremanagement` module uses Go 1.22. That
wasn't intentional. This patch changes it to use Go 1.21 and the same
dependencies than the rest of the project.

Related: openshift-kni#109

Signed-off-by: Juan Hernandez <[email protected]>

* NO-ISSUE: Move selector logic from handlers to adapter (openshift-kni#117)

Currently we apply the selector (the `filter` query parameter) in all
the handlers. This patch moves the logic to the adapter, which is where
it belongs.

Signed-off-by: Juan Hernandez <[email protected]>

* NO-ISSUE: remove redundant jsonAPI from handlers (openshift-kni#119)

Removed unsed `jsonAPI` from handlers (can be added ad-hoc when needed).

* NO-ISSUE: fix typos related to alarms (openshift-kni#112)

* Supports for alarm notification server: (openshift-kni#102)

- Create the alarm notification handler and alarm notification server.
   - Add user passed in optional namespace and configmap_name for subscription server and notification server
   - Manage alarm subscriptions in memory base on persist storage (configuration map populated by alarm subscription server).
   - Prebuilt subscription filter terms
   - Accept the alarts from alertManager, map the received alert to alarm, match the filters and build and send out notification packet based on matched subscription
   - Daniel E authored the alert to alarm mapping portion in this commit, and the same code used by alarm server.
   - Enhanced subscription handler

* CNF-13590: Use the vendor directory when building images (openshift-kni#128)

Description:
- remove the vendor directory from .gitignore and .containerignore
  so that it gets copied into the image with the rest of the
  content
- add -mod=vendor to go build commands

* Add repo owners

* Add .gitattributes (openshift-kni#130)

---------

Signed-off-by: Juan Hernandez <[email protected]>
Co-authored-by: Juan Hernández <[email protected]>
Co-authored-by: Daniel Erez <[email protected]>
Co-authored-by: Jennifer-chen-rh <[email protected]>
Co-authored-by: Irina Mihai <[email protected]>
openshift-merge-bot bot pushed a commit that referenced this pull request Aug 20, 2024
* Provisioning POC: Operator boilerplate

Description:
- Add the boilerplate for the operator provisioning logic:
  * ClusterTemplate & ClusterRequest CRs
  * ClusterTemplate & ClusterRequest controllers

* CNF-13444: ClusterTemplate CRD and controller logic

Description:
- add inputDataSchema to the ClusterTemplate CR
- add controller logic to validate the inputDataSchema
  is a valid JSON and reflect the validation results in
  the status of the clusterTemplate CR
- add unit tests

* CNF-13446: ClusterRequest CRD fields

Description:
- add the clusterTemplateRef and clusterTemplateInput to the
  ClusterRequest CRD to avoid future merge conflicts

* CNF-13446: Add and validate clusterTemplateInput

Description
- add validation for the clusterTemplateInput field of
  clusterRequest
- save validation status to the CR's status

* initial commit for siteconfig template cm

* CNF-13446: ClusterRequest schema match with template

Description:
- match the clusterTemplateInput with the inputDataSchema of the
  clusterTemplateRef
- automatically trigger clusterRequest reconciliation if
  clusterTemplates used by clusterRequests are updated or
  deleted
- unit tests

* ClusterInstance generation

Signed-off-by: Angie Wang <[email protected]>

* CNF-13448: Create resources needed by ClusterInstance

Description:
- create the namespace, BMS secrets, pull secret and
  extra-manifests ConfigMaps
- updates for the new ClusterInstance CRD

* Ensure the accurate merging of clusterTemplateInput with the default data

* CNF-13447-CNF-13449: Watches and finalizer

Description:
- add watches for the ClusterInstance and the ManagedCluster
- add finalizer for the ClusterRequest

* ClusterTemplate and ClusterRequest CRDs spec updates for ClusterInstance

* Change the type of clusterInstanceSchema and clusterInstanceInput to object

* CNF-13706: [POC2] ACM PolicyGenerator Schema

Description:
- sample for ACM policy generator that would be used in the
  ClusterTemplate spec.inputDataSchema.policyTemplateSchema
- the current sample is subject to change

* Add repo owners

* Add ClusterTemplate validation and status condition (#131)

Signed-off-by: Angie Wang <[email protected]>

* Resync POC branch with main (#136)

* MGMT-18232: Avoid `controller-gen` crash (#108)

Version of `controller-gen` 0.13.0 crashes like this when we execute
`make install`:

```
/home/user/repos/oran-o2ims/bin/controller-gen rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xa0de0f]goroutine 1 [running]:
go/types.(*Checker).handleBailout(0xc0004aa600, 0xc000e42a18)
        /usr/lib/golang/src/go/types/check.go:367 +0x88
panic({0xbc7860?, 0x12b48a0?})
        /usr/lib/golang/src/runtime/panic.go:770 +0x132
go/types.(*StdSizes).Sizeof(0x0, {0xdc35b8, 0x12bd080})
        /usr/lib/golang/src/go/types/sizes.go:228 +0x30f
go/types.(*Config).sizeof(...)
        /usr/lib/golang/src/go/types/sizes.go:333
go/types.representableConst.func1({0xdc35b8?, 0x12bd080?})
        /usr/lib/golang/src/go/types/const.go:76 +0x9e
go/types.representableConst({0xdc9990, 0x1287f80}, 0xc0004aa600, 0x12bd080, 0xc000e42188)
        /usr/lib/golang/src/go/types/const.go:92 +0x192
go/types.(*Checker).representation(0xc0004aa600, 0xc000f397c0, 0x12bd080)
        /usr/lib/golang/src/go/types/const.go:256 +0x65
go/types.(*Checker).implicitTypeAndValue(0xc0004aa600, 0xc000f397c0, {0xdc35e0, 0xc0000b2700})
        /usr/lib/golang/src/go/types/expr.go:375 +0x2d7
go/types.(*Checker).assignment(0xc0004aa600, 0xc000f397c0, {0xdc35e0, 0xc0000b2700}, {0xc9528c, 0x14})
        /usr/lib/golang/src/go/types/assignments.go:52 +0x2e5
go/types.(*Checker).initConst(0xc0004aa600, 0xc00138f1a0, 0xc000f397c0)
        /usr/lib/golang/src/go/types/assignments.go:126 +0x2c5
go/types.(*Checker).constDecl(0xc0004aa600, 0xc00138f1a0, {0xdc6200, 0xc0006f9dc0}, {0xdc6200, 0xc0006f9de0}, 0x0)
        /usr/lib/golang/src/go/types/decl.go:490 +0x311
go/types.(*Checker).objDecl(0xc0004aa600, {0xdcf120, 0xc00138f1a0}, 0x0)
        /usr/lib/golang/src/go/types/decl.go:191 +0xa49
go/types.(*Checker).packageObjects(0xc0004aa600)
        /usr/lib/golang/src/go/types/resolver.go:693 +0x4dd
go/types.(*Checker).checkFiles(0xc0004aa600, {0xc00027e870, 0x2, 0x2})
        /usr/lib/golang/src/go/types/check.go:408 +0x1a5
go/types.(*Checker).Files(...)
        /usr/lib/golang/src/go/types/check.go:372
sigs.k8s.io/controller-tools/pkg/loader.(*loader).typeCheck(0xc00023f440, 0xc000992e00)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/loader.go:286 +0x36a
sigs.k8s.io/controller-tools/pkg/loader.(*Package).NeedTypesInfo(0xc000992e00)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/loader.go:99 +0x39
sigs.k8s.io/controller-tools/pkg/loader.(*TypeChecker).check(0xc000f3b560, 0xc000992e00)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/refs.go:268 +0x2b7
sigs.k8s.io/controller-tools/pkg/loader.(*TypeChecker).Check(...)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/loader/refs.go:216
sigs.k8s.io/controller-tools/pkg/crd.(*Parser).AddPackage(0xc00108d2c0, 0xc000992e00)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/crd/parser.go:224 +0xfd
sigs.k8s.io/controller-tools/pkg/crd.(*Parser).NeedPackage(0xc00108d2c0, 0xc000992e00)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/crd/parser.go:242 +0xee
sigs.k8s.io/controller-tools/pkg/crd.Generator.Generate({0x0, 0x0, 0x0, {0x0, 0x0, 0x0}, 0x0, {0x0, 0x0}, {0x0, ...}}, ...)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/crd/gen.go:116 +0x128
sigs.k8s.io/controller-tools/pkg/genall.(*Runtime).Run(0xc000468ab0)
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/genall/genall.go:273 +0x23d
main.main.func1(0xc00050c300?, {0xc0002346e0?, 0x4?, 0xc8af70?})
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/cmd/controller-gen/main.go:176 +0x6a
github.com/spf13/cobra.(*Command).execute(0xc000202c08, {0xc0000361f0, 0x5, 0x5})
        /home/user/go/pkg/mod/github.com/spf13/[email protected]/command.go:940 +0x882
github.com/spf13/cobra.(*Command).ExecuteC(0xc000202c08)
        /home/user/go/pkg/mod/github.com/spf13/[email protected]/command.go:1068 +0x3a5
github.com/spf13/cobra.(*Command).Execute(...)
        /home/user/go/pkg/mod/github.com/spf13/[email protected]/command.go:992
main.main()
        /home/user/go/pkg/mod/sigs.k8s.io/[email protected]/cmd/controller-gen/main.go:200 +0x2f6
make: *** [Makefile:113: manifests] Error 2
```

I didn't find a bug report for this, but the latest version (0.15.0)
works correctly. This patch updates the project to use that version.
Note that that has some minor effects in the generated custom resource
definitions: the newer version uses `description: |-` instead of just
`description: ...`. That is also included in the patch.

Related: https://issues.redhat.com/browse/MGMT-18232

Signed-off-by: Juan Hernandez <[email protected]>

* NO-ISSUE: Use Go 1.21 in all modules (#110)

The recently added `api/hardwaremanagement` module uses Go 1.22. That
wasn't intentional. This patch changes it to use Go 1.21 and the same
dependencies than the rest of the project.

Related: #109

Signed-off-by: Juan Hernandez <[email protected]>

* NO-ISSUE: Move selector logic from handlers to adapter (#117)

Currently we apply the selector (the `filter` query parameter) in all
the handlers. This patch moves the logic to the adapter, which is where
it belongs.

Signed-off-by: Juan Hernandez <[email protected]>

* NO-ISSUE: remove redundant jsonAPI from handlers (#119)

Removed unsed `jsonAPI` from handlers (can be added ad-hoc when needed).

* NO-ISSUE: fix typos related to alarms (#112)

* Supports for alarm notification server: (#102)

- Create the alarm notification handler and alarm notification server.
   - Add user passed in optional namespace and configmap_name for subscription server and notification server
   - Manage alarm subscriptions in memory base on persist storage (configuration map populated by alarm subscription server).
   - Prebuilt subscription filter terms
   - Accept the alarts from alertManager, map the received alert to alarm, match the filters and build and send out notification packet based on matched subscription
   - Daniel E authored the alert to alarm mapping portion in this commit, and the same code used by alarm server.
   - Enhanced subscription handler

* CNF-13590: Use the vendor directory when building images (#128)

Description:
- remove the vendor directory from .gitignore and .containerignore
  so that it gets copied into the image with the rest of the
  content
- add -mod=vendor to go build commands

* Add repo owners

* Add .gitattributes (#130)

---------

Signed-off-by: Juan Hernandez <[email protected]>
Co-authored-by: Juan Hernández <[email protected]>
Co-authored-by: Daniel Erez <[email protected]>
Co-authored-by: Jennifer-chen-rh <[email protected]>
Co-authored-by: Irina Mihai <[email protected]>

* Update vendors with missing modules (#137)

Signed-off-by: Don Penney <[email protected]>

* NO-ISSUE: Add POC branch to github workflows

Signed-off-by: Don Penney <[email protected]>

* CNF-13894: Hardware provisioning POC (#135)

Signed-off-by: Tao Liu <[email protected]>

* CRDs updates due to upversioned controller-gen (#140)

* Set transport defaults when creating HTTP backend transport (#139)

The zero value of HttpTransport doesn't include any default attribute
values for setting up the HTTP transport.  This prevents using the
HTTPS_PROXY environment variable (and other related proxy variables) to
redirect HTTP requests to a proxy.

We could set this explicitly on our own, but that wouldn't pick up any
future changes to the defaults made upstream in the library.

This is useful when testing as a standalone binary and connecting to a
system that is behind a proxy.

Signed-off-by: Allain Legacy <[email protected]>

* Move NodePool to the hardwaremanagement Module Update 1 (#143)

Signed-off-by: Tao Liu <[email protected]>

* Add support for policy template configuration (#133)

* Exclude vendor directory from gofmt input parameters (#145)

Now that the `vendor` directory is no longer excluded from git we need
to adjust the `gofmt` input parameters to exclude it since it currently
looks at any and all *.go files within the entire repo.  This causes it
to run on files within the vendor directory which, as it turns out,
contains some *.go files that do not conform to the go formatting
guidelines.

Alternatively we could have explicitly listed the required source
directories, but figured that this dynamic approach would be more
"future-proof" and require less code maintenance.

Signed-off-by: Allain Legacy <[email protected]>

* Add alegacy to OWNERS_ALIASES (#147)

Signed-off-by: Don Penney <[email protected]>

* test: Avoid fake client server-side apply error (#141)

In the current k8s dependencies, the fake client does not support
server-side apply calls and returns an error in the ginkgo tests:

  [FAILED] Unexpected error:
      <*errors.errorString | 0xc00064e1a0>:
      apply patches are not supported in the fake client. Follow kubernetes/kubernetes#115598 for the current status
      {
          s: "apply patches are not supported in the fake client. Follow kubernetes/kubernetes#115598 for the current status",
      }
  occurred

This update adds a check for this error to bypass the failure, until the
k8s modules can be updated to a version that does not produce this
error.

Signed-off-by: Don Penney <[email protected]>

* Add unittests for policytemplate configmap (#151)

* NO-ISSUE: POC: workflow: Update golangci-lint to 1.59.1 (#150)

* workflow: Update golangci-lint to 1.59.1

Signed-off-by: Don Penney <[email protected]>

* Add initial golangci-lint config file

Signed-off-by: Don Penney <[email protected]>

---------

Signed-off-by: Don Penney <[email protected]>

* Enable golangci-lint misspell test (#152)

Signed-off-by: Don Penney <[email protected]>

* CNF-13957: Enable goimports linter configuration (#153)

This addresses import statement ordering/grouping violations reported by
the golintci tool.

Signed-off-by: Allain Legacy <[email protected]>

* CNF-13961: Enable unconvert linter configuration (#154)

This addresses issues with unnecessary type conversions reported by the
golintci tool.

Signed-off-by: Allain Legacy <[email protected]>

* Enable golangci-lint goconst test (#157)

Signed-off-by: Don Penney <[email protected]>

* Enable golangci-lint unparam test (#155)

Signed-off-by: Don Penney <[email protected]>

* CNF-13953: Enable errorlint linter configuration (#156)

This addresses issues with improperly unwrapped error objects reported
by the golintci tool.

Signed-off-by: Allain Legacy <[email protected]>

* CNF-13956: Enable gocyclo linter configuration (#158)

This enables the code complexity test and addresses the violations
reported by the golintci tool.

Signed-off-by: Allain Legacy <[email protected]>

* Enable golangci-lint gocritic test (#159)

Signed-off-by: Don Penney <[email protected]>

* Use dev script to pin golangci-lint version for users (#162)

Signed-off-by: Don Penney <[email protected]>

* golangci-lint: Fix some more gocritic lints (#161)

Signed-off-by: Leonardo Ochoa-Aday <[email protected]>

* Enable golangci-lint for _test.go files (#163)

Signed-off-by: Don Penney <[email protected]>

* Enable additional golangci-lint linters (#164)

The following golangci-lint linters are enabled:
- errcheck
- gosimple
- importas
- nilnil
- predeclared
- promlinter
- sloglint
- usestdlibvars

Signed-off-by: Don Penney <[email protected]>

* Import NodePool from the hardwaremanagement module (#160)

Signed-off-by: Tao Liu <[email protected]>

* Requeue and status handling in ClusterRequest controller (#144)

Signed-off-by: Angie Wang <[email protected]>

* CNF-13707: Policy template configMap updates (#165)

Description:
- update the policy template configMap keys to keep them the
  same as the default policy template configMap
- do not check for a policy label on the clusterInstance (the user
  needs to make sure the labels used in the ClusterInstance and the
  ACM PGs match)
- add unit tests

* Update Dockerfile to copy vendor folder as separate layer (#166)

By separating the vendor folder to its own COPY directive in the
Dockerfile, the build is able to make use of the cached directory more
efficiently, as the vendor files will change infrequently.

Signed-off-by: Don Penney <[email protected]>

* Add RBAC for nodepools (#167)

Signed-off-by: Tao Liu <[email protected]>

* CNF-13958: Enable gosec linter configuration (#169)

This enables the code security test and addresses the violations
reported by the golintci tool.

Signed-off-by: Allain Legacy <[email protected]>

* golangci-lint: Enable wrapcheck linter configs (#170)

Signed-off-by: Leonardo Ochoa-Aday <[email protected]>
Co-authored-by: Don Penney <[email protected]>

* CNF-14009: Node Hostname Update and BootMacAddress Retrieval (#168)

* CNF-14009: Node Hostname Update and BooMacAddress

Enhanced the IMS operator to update Node status with the hostname
post-BMC assignment and use the bootMacAddress from Node status
to update the ClusterInstance.

Signed-off-by: Tao Liu <[email protected]>

* CNF-14009: Node Hostname Update and BootMacAddress

Enhanced the IMS operator to update Node status with the hostname
post-BMC assignment and use the bootMacAddress from Node status
to update the ClusterInstance.

Signed-off-by: Tao Liu <[email protected]>

---------

Signed-off-by: Tao Liu <[email protected]>

* Update vendor folder

Signed-off-by: Don Penney <[email protected]>

* Fix merge issues

Signed-off-by: Don Penney <[email protected]>

---------

Signed-off-by: Angie Wang <[email protected]>
Signed-off-by: Juan Hernandez <[email protected]>
Signed-off-by: Don Penney <[email protected]>
Signed-off-by: Tao Liu <[email protected]>
Signed-off-by: Allain Legacy <[email protected]>
Signed-off-by: Leonardo Ochoa-Aday <[email protected]>
Co-authored-by: Irina Mihai <[email protected]>
Co-authored-by: Angie Wang <[email protected]>
Co-authored-by: Juan Hernández <[email protected]>
Co-authored-by: Daniel Erez <[email protected]>
Co-authored-by: Jennifer-chen-rh <[email protected]>
Co-authored-by: Tao Liu <[email protected]>
Co-authored-by: Allain Legacy <[email protected]>
Co-authored-by: Allain Legacy <[email protected]>
Co-authored-by: Leo Ochoa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants