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

icingadb-migrate: handle NULL event time columns #551

Merged
merged 1 commit into from
Jul 25, 2023
Merged

icingadb-migrate: handle NULL event time columns #551

merged 1 commit into from
Jul 25, 2023

Conversation

Al2Klimov
Copy link
Member

@Al2Klimov Al2Klimov commented Dec 13, 2022

i.e. ignore such rows.

Edit

This is the best only thing we can do here.

ScheduledStartTime int64
ScheduledStartTime sql.NullInt64
ScheduledEndTime int64
ActualStartTime int64
Copy link
Contributor

Choose a reason for hiding this comment

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

According to #550 (comment), actual_start_time also was NULL. Might be worth looking at the schema which columns are allowed to be NULL an handle all these.

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't worth it IMAO. Everything that has a reason to be NULL is already handled.

Copy link
Contributor

Choose a reason for hiding this comment

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

"a reason to be NULL" may also be "there was an old icinga2 version that had a bug that didn't populate that column"

Copy link
Member Author

Choose a reason for hiding this comment

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

*rolling eyes* s/reason/actual reason/

I'd rather wait for that bug also arriving here.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't worth it IMAO. Everything that has a reason to be NULL is already handled.

It's worth it. This bug report is proof of that.

I'd rather wait for that bug also arriving here.

I would rather fix them before. We know that almost everyone had or has an Icinga 2 version that has bugs 😆.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm afraid there could be NULL literally everywhere ex. IDs.

$ grep -Fwie 'NOT NULL' <lib/db_ido_mysql/schema/mysql.sql |grep -vFe _id
$

On the other hand there's only 1 report yet.

Still sure about the effort/gain ratio?

@Al2Klimov
Copy link
Member Author

As reaction to basically an additional report – #550 (comment) – I've handled ALL timestamps now.

@lippserd
Copy link
Member

lippserd commented Jun 5, 2023

@Al2Klimov Please adjust PR title and description.

@Al2Klimov Al2Klimov changed the title icingadb-migrate: handle NULL icinga_downtimehistory#scheduled_start_time icingadb-migrate: handle NULL event time columns Jun 5, 2023
@log1-c
Copy link

log1-c commented Jul 5, 2023

Compiled the icingadb-migrate from this PR and there are still errors. Assuming when inserting into the pgsql database:

2023-07-05T09:55:06.765+0200    INFO    icingadb-migrate/main.go:78     Starting IDO to Icinga DB history migration
2023-07-05T09:55:06.765+0200    INFO    icingadb-migrate/main.go:175    Connecting to databases
2023-07-05T09:55:06.770+0200    INFO    icingadb-migrate/main.go:141    Preparing cache
2023-07-05T09:55:06.789+0200    INFO    icingadb-migrate/main.go:93     Computing progress
2023-07-05T09:55:06.794+0200    INFO    icingadb-migrate/main.go:330    Counted migrated IDO events     {"type": "state", "migrated": 20000, "total": 3123737}
2023-07-05T09:55:06.794+0200    INFO    icingadb-migrate/main.go:330    Counted migrated IDO events     {"type": "downtime", "migrated": 0, "total": 7262}
2023-07-05T09:55:06.794+0200    INFO    icingadb-migrate/main.go:330    Counted migrated IDO events     {"type": "flapping", "migrated": 0, "total": 0}
2023-07-05T09:55:06.794+0200    INFO    icingadb-migrate/main.go:330    Counted migrated IDO events     {"type": "notification", "migrated": 13006, "total": 13006}
2023-07-05T09:55:06.794+0200    INFO    icingadb-migrate/main.go:330    Counted migrated IDO events     {"type": "ack & comment", "migrated": 747, "total": 747}
2023-07-05T09:55:06.794+0200    INFO    icingadb-migrate/main.go:103    Filling cache
flapping   0 %    0s  0/s
notification 100 %    0s  0/s
state 100 %    0s 15891/s
2023-07-05T09:57:47.804+0200    INFO    icingadb-migrate/main.go:106    Actually migrating
ack & comment 100 %    0s  0/s
downtime   0 % [------------------------------------------------------------------------------------------------------------------------------------------]   0s  0/s
flapping   0 %    0s  0/s
notification 100 %    0s  0/s
state   1 % [>--------------------------------------------------------------------------------------------------------------------------------------------]   0s  0/s
2023-07-05T09:57:49.350+0200    FATAL   icingadb-migrate/main.go:449    pq: null value in column "downtime_end" of relation "sla_history_downtime" violates not-null constraint
can't perform "INSERT INTO \"sla_history_downtime\" (\"downtime_end\", \"downtime_start\", \"downtime_id\", \"environment_id\", \"endpoint_id\", \"object_type\", \"host_id\", \"service_id\") VALUES (:downtime_end, :downtime_start, :downtime_id, :environment_id, :endpoint_id, :object_type, :host_id, :service_id) ON CONFLICT ON CONSTRAINT pk_sla_history_downtime DO NOTHING"
github.com/icinga/icingadb/internal.CantPerformQuery
        /home/t1-123/icingadb/internal/internal.go:30
github.com/icinga/icingadb/pkg/icingadb.(*DB).NamedBulkExec.func1.1.1.1
        /home/t1-123/icingadb/pkg/icingadb/db.go:394
github.com/icinga/icingadb/pkg/retry.WithBackoff
        /home/t1-123/icingadb/pkg/retry/retry.go:45
github.com/icinga/icingadb/pkg/icingadb.(*DB).NamedBulkExec.func1.1.1
        /home/t1-123/icingadb/pkg/icingadb/db.go:389
golang.org/x/sync/errgroup.(*Group).Go.func1
        /root/go/pkg/mod/golang.org/x/sync@v0.2.0/errgroup/errgroup.go:75
runtime.goexit
        /usr/lib/golang/src/runtime/asm_amd64.s:1594
can't retry
github.com/icinga/icingadb/pkg/retry.WithBackoff
        /home/t1-123/icingadb/pkg/retry/retry.go:64
github.com/icinga/icingadb/pkg/icingadb.(*DB).NamedBulkExec.func1.1.1
        /home/t1-123/icingadb/pkg/icingadb/db.go:389
golang.org/x/sync/errgroup.(*Group).Go.func1
        /root/go/pkg/mod/golang.org/x/sync@v0.2.0/errgroup/errgroup.go:75
runtime.goexit
        /usr/lib/golang/src/runtime/asm_amd64.s:1594
can't perform DML
main.migrateOneType[...].func3
        /home/t1-123/icingadb/cmd/icingadb-migrate/main.go:449
main.sliceIdoHistory[...]
        /home/t1-123/icingadb/cmd/icingadb-migrate/misc.go:135
main.migrateOneType[...]
        /home/t1-123/icingadb/cmd/icingadb-migrate/main.go:422
main.glob..func3
        /home/t1-123/icingadb/cmd/icingadb-migrate/misc.go:262
main.migrate.func1
        /home/t1-123/icingadb/cmd/icingadb-migrate/main.go:360
main.historyTypes.forEach.func1
        /home/t1-123/icingadb/cmd/icingadb-migrate/misc.go:232
golang.org/x/sync/errgroup.(*Group).Go.func1
        /root/go/pkg/mod/golang.org/x/sync@v0.2.0/errgroup/errgroup.go:75
runtime.goexit
        /usr/lib/golang/src/runtime/asm_amd64.s:1594    {"backend": "Icinga DB", "op": "INSERT IGNORE", "table": "sla_history_downtime"}
main.migrateOneType[...].func3
        /home/t1-123/icingadb/cmd/icingadb-migrate/main.go:449
main.sliceIdoHistory[...]
        /home/t1-123/icingadb/cmd/icingadb-migrate/misc.go:135
main.migrateOneType[...]
        /home/t1-123/icingadb/cmd/icingadb-migrate/main.go:422
main.glob..func3
        /home/t1-123/icingadb/cmd/icingadb-migrate/misc.go:262
main.migrate.func1
        /home/t1-123/icingadb/cmd/icingadb-migrate/main.go:360
main.historyTypes.forEach.func1
        /home/t1-123/icingadb/cmd/icingadb-migrate/misc.go:232
golang.org/x/sync/errgroup.(*Group).Go.func1
        /root/go/pkg/mod/golang.org/x/sync@v0.2.0/errgroup/errgroup.go:75

@Al2Klimov
Copy link
Member Author

Good catch! But as far as I see, history.SlaHistoryDowntime#EndTime is always non-nil there. However, the struct field itself misses the correct DB column.

@log1-c
Copy link

log1-c commented Jul 5, 2023

created this PR with more changes, with those the migrate worked without issues:
#613

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